Skip to content

Hex+aes #71

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Hex+aes #71

wants to merge 5 commits into from

Conversation

scr-oath
Copy link
Collaborator

No description provided.

@scr-oath scr-oath mentioned this pull request Jun 24, 2025
Merged
dborovcanin
dborovcanin approved these changes Jun 25, 2025
View reviewed changes
Copy link
Contributor

@dborovcanin dborovcanin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some more details to the README, so these two approaches are better documented. Overall, this looks good to me.

@scr-oath
Copy link
Collaborator Author

scr-oath commented Jun 25, 2025 •
edited
Loading

Please add some more details to the README, so these two approaches are better documented. Overall, this looks good to me.

Nice - I actually found some bugs in the README - since you're calling ToString and not doing a switch on the type - passing 1 would not become the GCM constant, it would fail to lookup in the mapping. I changed the example to agree. We could also fix that by adding 1,2,3 as mappings… hmm… on second thought - passing numbers isn't very meaningful… I would rather leave it as is and the documentation agrees with what string values can be sent.

I added some "constants" (members of the crypto) that can be used rather than bare strings or numbers.

@scr-oath scr-oath marked this pull request as ready for review June 25, 2025 23:47
@scr-oath scr-oath requested a review from vadv June 25, 2025 23:47
@scr-oath scr-oath requested a review from dborovcanin June 25, 2025 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dborovcanin dborovcanin dborovcanin approved these changes

@vadv vadv Awaiting requested review from vadv

At least 1 approving review is required to merge this pull request.

Assignees

@vadv vadv

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@scr-oath @dborovcanin @vadv