Skip to content

Conversation

@tgfukuda
Copy link
Contributor

@tgfukuda tgfukuda commented Sep 6, 2022

Motivation

#2975

Solution

Change logged address (input, output, ...) to checksummed with ethers::core::utils::to_checksum.
It converts Address to String, so including fix for some formatting.

still not fixing broadcast json file, WIP

This is my first PR, so I'm sorry if it's in a wrong manner.

Copy link
Member

@gakonst gakonst left a 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?

@tgfukuda
Copy link
Contributor Author

tgfukuda commented Sep 8, 2022

@gakonst

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.
https://github.com/gakonst/ethers-rs/blob/master/ethers-core/src/utils/mod.rs#L279-L298
The checksummed address as EIP-1191 is possible but it may require many changes because some of the contexts like https://github.com/foundry-rs/foundry/pull/3108/files#diff-2a68e14bf95d2e3c7c7c72b92741c10dc2ea9027388a899a65a9a73b4152b80fR504 doesn't have chainId.
I like for PR to be as small as possible, so I think it is enough to checksum address in an EIP-55 style when it comes to this PR.
I don't know which one is better and want comments for it.

Copy link
Member

@gakonst gakonst left a 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?

@tgfukuda tgfukuda force-pushed the checksum-address-output branch from 5a4c6e3 to 920cf24 Compare September 10, 2022 17:01
@tgfukuda
Copy link
Contributor Author

@gakonst
Thank you for your review.

I added the test for format_token method.

Additionally, addresses in a broadcast log of forge script becomes checksummed except for TypedTransaction of TransactionMetadata.
TypedTransaction is enum and the changes may be too large to review, I think. So it is not enough to the goal of this PR.

Log file generated by forge script is deeply depends on serde and ethers-rs implementation. To checksum every address of logs, need to modify the Serialize implementation of every struct related to it.

@tgfukuda tgfukuda marked this pull request as ready for review September 10, 2022 18:32
Copy link
Member

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

sweet

@gakonst gakonst merged commit 1d1e182 into foundry-rs:master Sep 10, 2022
@gakonst
Copy link
Member

gakonst commented Sep 10, 2022

thank you for the contribution @tgfukuda! very helpful.

iFrostizz pushed a commit to iFrostizz/foundry that referenced this pull request Nov 9, 2022
…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>
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.

2 participants