NDRS-299 client wasmless transfer#184
Conversation
Fraser999
requested review from
EdHastingsCasperAssociation,
afck,
fizyk20,
goral09,
marc-casperlabs and
mpapierski
afck
left a comment
afck
left a comment
There was a problem hiding this comment.
Looks good to me, but someone more familiar with the client code should review it, too.
client/src/deploy/creation_common.rs
Outdated
| values | ||
| .map(|hex_hash| { | ||
| let digest = Digest::from_hex(hex_hash) | ||
| .unwrap_or_else(|error| panic!("should parse {}: {}", ARG_NAME, error)); |
There was a problem hiding this comment.
Since this is a user input error, maybe:
panic!("could not parse --{} {} as hex-encoded deploy hash: {}", ARG_NAME, hex_hash, error)
node/src/types/deploy.rs
Outdated
| approvals, | ||
| }; | ||
|
|
||
| if let Err(error) = deploy.verify_approval(deploy.header.account(), Some(0)) { |
There was a problem hiding this comment.
What are the other approvals for if we only verify the first one?
There was a problem hiding this comment.
I'm, not sure what the plan is in that regard. This provides similar functionality to that in the Python client, i.e. the ability for multiple signers to sign a given deploy. I imagine this will be built upon in the future. Either we'll need to include the signers' public keys in the deploy to enable verification during deserialization, or else it'll need to be done separately to deserialization, or else it might never need validated. Either way, I believe that'll need further planning.
There was a problem hiding this comment.
In Scala we are verifying all signatures – so yes, it requires including signer's public key as well. Execution Engine assumes that node has verified that all signatures are valid when it checks whether deploy is allowed for execution. This is important in the multisignature context.
There was a problem hiding this comment.
Thanks @goral09 - I'll add that here too.
47: Regression test for validation of empty blocks with accusations. r=Fraser999 a=afck Test for https://github.com/casper-network/casper-node-private/pull/46. This passed 3 out of 3 times for me. Without the fix, it failed 3 out of 3 times. Closes [#184](https://github.com/casper-network/Governance/issues/184). Co-authored-by: Andreas Fackler <andreas@casperlabs.io> Co-authored-by: TomVasile <43349666+TomVasile@users.noreply.github.com>
As pointed by @mediremi , we might overflow on the post-execution gas deduction. That operation must be safe, so we have to use `checked_sub`
4 participants
This PR adds a
--transfersubcommand to the client which leverages the wasmless transfer functionality provided by the contract runtime.It also sees the
Deploytype cleaned up significantly, with validation of hashes now performed during deserialization, and better error reporting.