-
Notifications
You must be signed in to change notification settings - Fork 2.2k
allowing the option of choosing between "input" and "data" field #11375
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
base: master
Are you sure you want to change the base?
allowing the option of choosing between "input" and "data" field #11375
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.
i don't know if this is something that we want to implement, cause i'm unaware of this behavior, so i'll defer that decision to others. cc: @zerosnacks @grandizzy
however, first you need to improve both the impl and the tests.
crates/cast/src/cmd/access_list.rs
Outdated
/// by default both are populated. Set this to populate one or the | ||
/// other. | ||
#[arg(long = "use-explicit-data-field", help_heading = "explicitly use \"input\" or \"data\" when calling an rpc where required")] | ||
pub use_explicit_data_field: Option<String>, |
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 API is weak, this should be an enum:
#[derive(Debug, Clone, Copy, Default, ValueEnum)]
enum TxDataField {
Input,
Data,
#[default]
Both,
}
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.
ok, I was debating between the two, I will change it to an enum 👍
self.tx.input = TransactionInput { input: Some(input.clone()), data: Some(input) }; | ||
|
||
match self.use_explicit_data_field { | ||
Some(ref u) => { | ||
match u.as_ref() { | ||
"input" => self.tx.input.data = Option::None, | ||
"data" => self.tx.input.input = Option::None, | ||
_ => { | ||
eyre::bail!( | ||
format!("invalid value for data field: {}", u) | ||
) | ||
} | ||
} | ||
}, | ||
None => {}, | ||
} |
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.
match against the enum.
it doesn't make sense to set both fields first, and then set one to none... instead it should be properly done directly, at once.
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.
will update 👍
crates/cast/src/cmd/access_list.rs
Outdated
/// When calling an RPC with optionally an "input" or a "data" field, | ||
/// by default both are populated. Set this to populate one or the | ||
/// other. |
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.
imo, this cmnt should document which RPCs prevent "input" and "data" from being set simultaneously.
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.
ok, I'll add that. with my work, it was the "forking" functionality in hardhat. I'll add that detail here
crates/cast/tests/cli/main.rs
Outdated
casttest!(access_list_explicit_data_field_input, |_prj, cmd| { | ||
let rpc = next_http_rpc_endpoint(); | ||
cmd.args([ | ||
"access-list", | ||
"0xbb2b8038a1640196fbe3e38816f3e67cba72d940", | ||
"skim(address)", | ||
"0xbb2b8038a1640196fbe3e38816f3e67cba72d940", | ||
"--rpc-url", | ||
rpc.as_str(), | ||
"--gas-limit", // need to set this for alchemy.io to avoid "intrinsic gas too low" error | ||
"100000", | ||
"--use-explicit-data-field", | ||
"input" | ||
]) | ||
.assert_success(); | ||
}); | ||
|
||
casttest!(access_list_explicit_data_field_data, |_prj, cmd| { | ||
let rpc = next_http_rpc_endpoint(); | ||
cmd.args([ | ||
"access-list", | ||
"0xbb2b8038a1640196fbe3e38816f3e67cba72d940", | ||
"skim(address)", | ||
"0xbb2b8038a1640196fbe3e38816f3e67cba72d940", | ||
"--rpc-url", | ||
rpc.as_str(), | ||
"--gas-limit", // need to set this for alchemy.io to avoid "intrinsic gas too low" error | ||
"100000", | ||
"--use-explicit-data-field", | ||
"data" | ||
]) | ||
.assert_success(); | ||
}); | ||
|
||
casttest!(access_list_explicit_data_field_invalid, |_prj, cmd| { | ||
let rpc = next_http_rpc_endpoint(); | ||
cmd.args([ | ||
"access-list", | ||
"0xbb2b8038a1640196fbe3e38816f3e67cba72d940", | ||
"skim(address)", | ||
"0xbb2b8038a1640196fbe3e38816f3e67cba72d940", | ||
"--rpc-url", | ||
rpc.as_str(), | ||
"--gas-limit", // need to set this for alchemy.io to avoid "intrinsic gas too low" error | ||
"100000", | ||
"--use-explicit-data-field", | ||
"blah" | ||
]) | ||
.assert_failure().stderr_eq(str![[r#" | ||
Error: invalid value for data field: blah | ||
"#]]); | ||
}); | ||
|
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.
these tests only check that data validation works (which could by easily checked with some unit tests for the enum).
a proper test should verify that the payload (you didn't include any calldata) doesn't have both fields informed when the flag is set, otherwise you don't prove that the feature works
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.
ok, I was looking for ways to check the rpc request's body in the tests, I didn't see any way to. I think the best way to test would be to ensure that input
and data
are included/omitted properly in the json body of the http request. I didn't see a way to do that. I'll admit I was in a bit of a rush when I submitted this, so I will take another look now 🤔 I may reach back out if I can't find it.
hey @0xrusowsky , thanks for the replies and the feedback. I'll address them. For me, the "hardhard fork-a-network" functionality is where I had ran into this issue. So I wanted this change to be backwards compatible (i.e. if you never set this flag, this change wouldn't be noticed) but usable so I can work with the "hardhat fork node". |
im honestly losing it with this which chain/node is doing this @ClaytonNorthey92 ? and whats the error message |
crates/cast/src/cmd/call.rs
Outdated
/// by default both are populated. Set this to populate one or the | ||
/// other. | ||
#[arg(long = "use-explicit-data-field", help_heading = "explicitly use \"input\" or \"data\" when calling an rpc where required")] | ||
pub use_explicit_data_field: Option<String>, |
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.
for this we have
let me add a fromstr impl for this real quick so that we can use this as type here for clap
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.
thanks @mattsse , once that is merged I can rebase this PR on it. I'll address the other feedback in the meantime 👍
hey @mattsse ,it's the "hardhart fork a network". this is the error cast send --rpc-url http://localhost:998[882/1648]a958998408dd7695c8965c46bdabbc3003 0xff2dd5a10000000000000000000000000000000000000000000000000000000000000020000000000000000000000
0000000000000000000000000000000000000000001000000000000000000000000fa73580f4d72294ae9ee3daac36d8bf111b37ce900000000000000000000000
0c43ed1e8d70d0e5801514833fad3d93ba16da4aa0000000000000000000000000000000000000000000000000000000000000000 --private-key <...> -vvvv
Error: Failed to estimate gas: server returned an error response: error code -32602: duplicate field `data` at line 1 column 538,
data: {"message":"duplicate field `data` at line 1 column 538","data":{"method":"eth_estimateGas","params":[{"from":"0xa0ee7a142d2
67c1f36714e4a8f75612f20a79720","to":"0x382d0aa958998408dd7695c8965c46bdabbc3003","maxFeePerGas":"0xf","maxPriorityFeePerGas":"0x1"
,"input":"0xff2dd5a100000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000
000000000000000001000000000000000000000000fa73580f4d72294ae9ee3daac36d8bf111b37ce9000000000000000000000000c43ed1e8d70d0e5801514833
fad3d93ba16da4aa0000000000000000000000000000000000000000000000000000000000000000","data":"0xff2dd5a1000000000000000000000000000000
00000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000001000000000000000000000000fa73580f
4d72294ae9ee3daac36d8bf111b37ce9000000000000000000000000c43ed1e8d70d0e5801514833fad3d93ba16da4aa0000000000000000000000000000000000
000000000000000000000000000000","nonce":"0x57f","chainId":"0xaa36a7"},"pending"]}} |
very inconvenient -.- |
Tell me about it... 🙄 |
6183b1e
to
152e0d0
Compare
hey @0xrusowsky , I have addressed the following:
I have not included @mattsse 's changes in is there anything additional or anything that I have missed? |
i haven't worked in cast yet, so idk if there may be some helper method to check the payloads without the extra dependancy that u had to introduce (@mattsse please tell us if its acceptable), but the changes look way better than before now! with that being said, can we also include matt's changes on alloy please? |
@0xrusowsky yup I will add @mattsse 's changes shortly and re-tag you both when done 👍 |
hey @mattsse , could you create a new release in |
hey @mattsse @0xrusowsky , I was testing in pulling in the latest in would you both be willing to merge this? and once we update the |
When calling certain eth RPCs, they validate that "input" and "data" are not both set. As of now, `cast` will set both fields unconditionally, leading to this error with some nodes. Add a `--use-explicit-data-field` flag to choose between "input" and "data". If this flag is omitted, or the string value is not one of those two, revert to original functionality of both fields.
* added tests that assert input/data are in json bodies of expected requests * removed flag from where it's not used; access-list and estimate
* updated --use-explicit-data-field to be an enum of type TxDataField
* update comment to include example of nodes that have this issue that this PR addresses
becc077
to
5e4e20e
Compare
hey @mattsse and @0xrusowsky , sorry to bother you, I just wanted to bump my last message in case it was missed ☝️ |
@ClaytonNorthey92 there'll be a new release shortly, I consider this pr complete and can take care of finalizing it once release is out |
@mattsse great thanks! if there is anything further that I can do please let me know 👍 |
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.
@ClaytonNorthey92 I left one suggestion that would help wrapping this up quickly if we can already prepare the upcoming release
it doesnt have to compile just yet but getting rid of the then redundant TxDataField type would be helpful
crates/cast/src/cmd/mktx.rs
Outdated
/// other. Some nodes do not allow both to exist, hardhat "fork a network" | ||
/// is an example. | ||
#[arg(long = "use-explicit-data-field", help_heading = "explicitly use \"input\" or \"data\" when calling an rpc where required")] | ||
pub use_explicit_data_field: Option<TxDataField>, |
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.
can we rename these fields to input_kind: TransactionInputKind
hey @mattsse I pushed up a commit that changes I left the added enum in, but I left a comment here to replace it with the new alloy release's type. that way once the release is ready, we can get rid of this definition and just add the appropriate imports/ |
Motivation
When calling certain eth RPCs, they validate that "input" and "data" are not both set. As of now,
cast
will set both fields unconditionally, leading to this error with some nodes.this gives a way to address these, as the
--trace
flag is not always a way to "get around" the issue as mentioned in this closed issue.Solution
Add a
--use-explicit-data-field
flag to choose between "input" and "data". If this flag is omitted,or the string value is not one of those two, revert to original functionality of both fields.PR Checklist