-
Notifications
You must be signed in to change notification settings - Fork 83
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
Zero malleable fields before execution #678
Conversation
* Don't update tx.receiptsRoot after pushing receipts * Remove some now-obsolete GTF getters
The current version clears |
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.
The code looks much cleaner=DDD I like it. Thank you!
if let Some(s) = tx.as_script_mut() { | ||
*s.receipts_root_mut() = Default::default(); | ||
} | ||
tx.inputs_mut() | ||
.iter_mut() | ||
.for_each(fuel_tx::Input::prepare_init_execute); | ||
tx.outputs_mut() | ||
.iter_mut() | ||
.for_each(fuel_tx::Output::prepare_init_execute); |
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 want to call prepare_init_execute
, you can make it part of the ExecutableTransaction
=)
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.
Adding prepare_init_execute
to ExecutableTransaction
would mean adding field::ReceiptRoot
bound, which is not satisfied by the Create
transaction. Although, I'm not sure why Create
is an ExecutableTransaction
since there's nothing to execute there.
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.
You don't need to define the implementation inside of the ExecutableTransaction
. You can have a separate implementation for Create
and Script
transactions
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.
It feels a bit inelegant, as the method is never called for Create
. But it's still probably nicer than the current duplication. Done in 6a39361
Spec PR: FuelLabs/fuel-specs#545
Related to: FuelLabs/fuel-core#1620
Closes: #651
Also:
Before merging make sure that the spec pr above agrees with the changes here.