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 --from-rlp & --to-rlp #1465

Merged
merged 12 commits into from Jun 7, 2022
Merged

feat(cast): add --from-rlp & --to-rlp #1465

merged 12 commits into from Jun 7, 2022

Conversation

0xvv
Copy link
Contributor

@0xvv 0xvv commented May 1, 2022

Implementation of feature #1433

  • --to-rlp and --from-rlp for single item
  • rlp for lists
  • rlp for nested lists
  • strip and add 0x to hex
  • accepting json type array as input

@onbjerg
Copy link
Member

onbjerg commented May 2, 2022

flag to toggle adding 0x to hex output ?

I don't think a flag to add it makes a lot of sense since you can just do "0x$(cast --to-rlp ...)"

@onbjerg onbjerg added the T-feature Type: feature label May 2, 2022
@tynes
Copy link
Contributor

tynes commented May 2, 2022

flag to toggle adding 0x to hex output ?

I don't think a flag to add it makes a lot of sense since you can just do "0x$(cast --to-rlp ...)"

Should it always return 0x prefixed output for --to-rlp to match the behavior of other commands?

$ cast --to-uint256 0
0x0000000000000000000000000000000000000000000000000000000000000000

Definitely would like if --from-rlp worked on 0x prefixed strings as well! Thank you for this :)

@onbjerg onbjerg linked an issue May 2, 2022 that may be closed by this pull request
@0xvv 0xvv force-pushed the rlp branch 3 times, most recently from e6c3477 to 47dcb39 Compare May 2, 2022 21:40
@gakonst
Copy link
Member

gakonst commented May 6, 2022

thx for this @0xvv, wonder what your plans are for getting this over the line :D

@0xvv
Copy link
Contributor Author

0xvv commented May 9, 2022

lil update, encoding is working but i still have quite a lot of work on decoding and arguments parsing

@0xvv
Copy link
Contributor Author

0xvv commented May 25, 2022

I'm still gonna add some finishing touches but most of the code is here, any feedback welcome.

Also what is the better way to handle string or hex input in the cli ? i don't think having 4 flags is the best, is the --from-rlp even useful ? I kinda mindlessly added it but is there really a use case ?

Here are some usage examples for reference

Screenshot from 2022-05-25 23-52-47

@0xvv 0xvv force-pushed the rlp branch 2 times, most recently from d7c62db to f8988bc Compare May 25, 2022 22:19
@0xvv 0xvv marked this pull request as ready for review May 26, 2022 15:58
@0xvv 0xvv changed the title cast: add --from-rlp & --to-rlp feat(cast): add --from-rlp & --to-rlp May 26, 2022
@0xvv
Copy link
Contributor Author

0xvv commented May 26, 2022

ℹ️ Just renamed --from-rlp-hex to --hex-from-rlp for consistency

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.

thanks for this,

couple of smol nits

cast/Cargo.toml Outdated Show resolved Hide resolved
cast/src/rlp_converter.rs Outdated Show resolved Hide resolved
cast/src/rlp_converter.rs Outdated Show resolved Hide resolved
cast/src/rlp_converter.rs Outdated Show resolved Hide resolved
cast/src/rlp_converter.rs Outdated Show resolved Hide resolved
cast/src/rlp_converter.rs Outdated Show resolved Hide resolved
Copy link
Member

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

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

Some minor nits (feel free to skip) but also some questions about the functionality

cast/src/lib.rs Outdated Show resolved Hide resolved
cast/src/lib.rs Outdated Show resolved Hide resolved
cast/src/lib.rs Outdated Show resolved Hide resolved
cast/src/rlp_converter.rs Outdated Show resolved Hide resolved
cast/src/rlp_converter.rs Outdated Show resolved Hide resolved
cli/src/opts/cast.rs Outdated Show resolved Hide resolved
cli/src/opts/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
cast/src/rlp_converter.rs Outdated Show resolved Hide resolved
@0xvv 0xvv requested a review from onbjerg May 28, 2022 20:21
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.

thanks!

this looks complete, mind adding casttest! to test these?

see https://github.com/foundry-rs/foundry/blob/master/cli/tests/it/cast.rs

@mattsse mattsse added the C-cast Command: cast label Jun 5, 2022
@0xvv 0xvv force-pushed the rlp branch 2 times, most recently from dd03912 to 3766796 Compare June 6, 2022 22:20
@0xvv
Copy link
Contributor Author

0xvv commented Jun 7, 2022

thanks!

this looks complete, mind adding casttest! to test these?

Added a couple, i can add some more cases if needed

@gakonst gakonst merged commit 564f0c4 into foundry-rs:master Jun 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cast Command: cast T-feature Type: feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cast --from-rlp and cast --to-rlp
5 participants