-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
fix(cast): duplicate data field #9558
base: master
Are you sure you want to change the base?
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 why a server would respond with a duplicate data field in this situation, do you know which client that is?
tx.input = TransactionInput { | ||
input: if tx_opts.input_only_data { None } else { Some(Bytes::new()) }, | ||
data: if tx_opts.input_only_input { None } else { Some(Bytes::new()) }, | ||
}; |
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 feels a bit weird because this now set the input/data field to empty bytes,
I believe this would technically be equivalent to null as well, just feels odd
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.
Yeah, that's right, the reason is I don't want to make too many changes, so I set the fields to empty bytes or null, then we can check empty bytes or null when we building transaction rather than pass additional arguments
/// Transaction input containing only the input field. | ||
/// By default, both the input and data fields will be set. | ||
#[arg(long, default_value = "false")] | ||
pub input_only_input: bool, | ||
|
||
/// Transaction input containing only the data field. | ||
/// By default, both the input and data fields will be set. | ||
#[arg(long, default_value = "false")] | ||
pub input_only_data: bool, |
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 really like introducing additional fields for this behaviour, the only reason why we even set both fields is because some nodes are off spec and expect the legacy data field...
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.
Totally agree, but now the node refuse the request if we se both fields.
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.
Maybe we can only keep input_only_input
because new nodes only accept the tx.input
.
Here are the version I'm using:
Here is the response from the node
I guess the hardhat refuse the request because we both send |
Motivation
fix the issue: #9327
Solution
Add two Transaction options: