Skip to content
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.

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

Ethereum address checksum encoding #656

Merged
merged 6 commits into from
Jun 6, 2023
Merged

Ethereum address checksum encoding #656

merged 6 commits into from
Jun 6, 2023

Conversation

RebeccaSelvaggini
Copy link
Collaborator

No description provided.

@jaromil
Copy link
Member

jaromil commented Jun 4, 2023

Well done!!! Can you also clearly state in the documentation that for checking the validity of an imported ethereum address one needs to read it also as string?

Given I have a 'string' named 'ethereum address' 
When I verify the ethereum address 'ethereum address' is valid

This is perhaps a good start for an example (taken from the test unit). The documentation should show a real-case scenario in which an address is imported, checked and then used to create a transaction. Reading the code now I'm not sure if its the case that the address needs to be imported twice, as schema and as string...

@jaromil
Copy link
Member

jaromil commented Jun 4, 2023

last force push is just the conflict resolution for minor conflicts caused by recent changes merged in master.

By doing so I have broken something in the export, sorry! can you find out a fix? after this last test is fixed this can be merged, the documentation task is addressed in #664 and can be developed in a different PR

@jaromil
Copy link
Member

jaromil commented Jun 6, 2023

well done!!!

@jaromil jaromil merged commit fded495 into master Jun 6, 2023
@jaromil jaromil deleted the eth-checksum branch June 6, 2023 08:56
@matteo-cristino matteo-cristino mentioned this pull request Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants