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

Feat(cast): Add --parse-bytes32-address #4746

Merged
merged 9 commits into from
Jun 9, 2023
Merged

Feat(cast): Add --parse-bytes32-address #4746

merged 9 commits into from
Jun 9, 2023

Conversation

0xSileo
Copy link
Contributor

@0xSileo 0xSileo commented Apr 15, 2023

Motivation

Add the --parse-bytes32-address option to cast. Earlier today, I was reading storage values and couldn't find a way to parse them into a checksummed address, i.e. go from this : 0x000000000000000000000000d8da6bf26964af9d7eed9e03e53415d37aa96045 to this : 0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045.

Solution

Implemented it

Other

I'm new to Rust and contributing (and quite proud of this PR actually), so feel free to provide any feedback.

@0xSileo 0xSileo changed the title Feat: Add --parse-bytes32-address Feat(cast): Add --parse-bytes32-address Apr 15, 2023
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

ty, smol nit.

would appreciate a book pr once merged

cli/tests/it/cast.rs Outdated Show resolved Hide resolved
cast/src/lib.rs Outdated Show resolved Hide resolved
cast/src/lib.rs Outdated Show resolved Hide resolved
@0xSileo 0xSileo requested a review from mattsse April 16, 2023 19:29
@tynes
Copy link
Contributor

tynes commented Apr 16, 2023

Perhaps a more generic type to type converter would be more useful? Going from uint256 to bytes32 or vice versa is similar and adding commands for each possible conversion is not ideal

@mds1
Copy link
Collaborator

mds1 commented Apr 16, 2023

Yea I had the same thought as @tynes, I'd suggest cast parse <fromType> <toType> <value>. For example:

$ cast parse bytes32 address 0x000000000000000000000000d8da6bf26964af9d7eed9e03e53415d37aa96045
0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045

cast convert could be another good name. IMO the current --from-* and --to-* flags are quite cluttered and hard to keep track of. I'd prefer to to simplify them with a generic function instead of adding to that list.

@mds1
Copy link
Collaborator

mds1 commented Apr 16, 2023

Also just noting you can achieve this currently using the --abi-decode method with a dummy signature, such as:

$ cast --abi-decode "x()(address)" 0x000000000000000000000000d8da6bf26964af9d7eed9e03e53415d37aa96045
0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045

@tynes
Copy link
Contributor

tynes commented Apr 17, 2023

Also just noting you can achieve this currently using the --abi-decode method with a dummy signature, such as:

$ cast --abi-decode "x()(address)" 0x000000000000000000000000d8da6bf26964af9d7eed9e03e53415d37aa96045
0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045

Definitely prefer adding this to the docs and making it the "canonical" solution to this problem, or cast convert. Reasoning is to not clutter the tool

@0xSileo
Copy link
Contributor Author

0xSileo commented Apr 17, 2023

Also just noting you can achieve this currently using the --abi-decode method with a dummy signature, such as:

$ cast --abi-decode "x()(address)" 0x000000000000000000000000d8da6bf26964af9d7eed9e03e53415d37aa96045
0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045

I thought about doing something like this, as I knew about specifying output types (e.g. "totalSupply()(uint)") but couldn't figure out a way to do it.

The abi-decode way of doing it works but is a bit hacky imo. Therefore, I'm in favor of adding a convert command, which would be more general and could remove a certain amount of clutter, as stated by both of you.

Should we open a new issue and close this PR (or merge it while the convert is not implemented) ?

I'll try to implement convert, seems challenging but doable for me.

@mds1
Copy link
Collaborator

mds1 commented Apr 17, 2023

Should we open a new issue [for convert] and close this PR

Personally this would be my preferred approach, @mattsse @tynes lmk if you have any objections

sorry for the churn here @51730 :)

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

I'm fine with this, if we want a more generic converter this can be extra

@mattsse mattsse merged commit f2a61d8 into foundry-rs:master Jun 9, 2023
18 of 19 checks passed
@0xSileo 0xSileo deleted the feature/bytes32ToAddress branch July 27, 2023 14:10
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.

None yet

4 participants