-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Checksum address output in log_address event and broadcast files #3108
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be fine, is it OK that we're skipping the chain id? guess so?
ethers-rs computes it as EIP-1191 if chainId is specified and as EIP-55 if not. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sgtm re: chain id. makes sense to leave as-is.
this is coming along nicely - maybe if we could add a test to ensure we don't break it later?
5a4c6e3 to
920cf24
Compare
|
@gakonst I added the test for Additionally, addresses in a broadcast log of Log file generated by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sweet
|
thank you for the contribution @tgfukuda! very helpful. |
…ndry-rs#3108) * checksumming log addr * fix lint * checksum addresses in broadcast's json file without transaction request metadata * add comments * add test for format_token in the case of address * fix lint Co-authored-by: tgfukuda <luktiger793@gmail.com>
Motivation
#2975
Solution
Change logged address (input, output, ...) to checksummed with
ethers::core::utils::to_checksum.It converts
AddresstoString, so including fix for some formatting.still not fixing
broadcastjson file, WIPThis is my first PR, so I'm sorry if it's in a wrong manner.