pallet-revive: EXTCODEHASH to match EIP-1052 #6088
Conversation
ermalkaleci
changed the title
substrate/frame/revive/src/exec.rs
Outdated
| // ALICE is not a contract and hence they do not have a code_hash | ||
| assert!(ctx.ext.code_hash(&ALICE_ADDR).is_none()); | ||
| assert!(ctx.ext.code_hash(&ALICE_ADDR).is_zero()); |
There was a problem hiding this comment.
The comment is wrong. Or the test.
There was a problem hiding this comment.
This is all mocked and easily to get confused. I was trying to minimise changes :)
substrate/frame/revive/src/exec.rs
Outdated
| // BOB is a contract and hence it has a code_hash | ||
| assert!(ctx.ext.code_hash(&BOB_ADDR).is_some()); | ||
| assert!(!ctx.ext.code_hash(&BOB_ADDR).is_zero()); |
There was a problem hiding this comment.
This should compare against an actual value. Not being zero means it can either be a non-contract account or a contract account
There was a problem hiding this comment.
Should also aim for testing both cases
substrate/frame/revive/src/exec.rs
Outdated
| @@ -1859,7 +1866,7 @@ mod tests { | |||
| ) -> H256 { | |||
| Loader::mutate(|loader| { | |||
| // Generate code hashes as monotonically increasing values. | |||
| let hash = <Test as frame_system::Config>::Hash::from_low_u64_be(loader.counter); | |||
| let hash = <Test as frame_system::Config>::Hash::from_low_u64_be(loader.counter + 1); | |||
There was a problem hiding this comment.
In the context of this PR it is obvious why this is needed. But out of context I don't think it's obvious too. Can you please update the comment above?
xermicus
added
R0-silent
| @@ -244,11 +244,7 @@ pub trait HostFn: private::Sealed { | |||
| /// | |||
| /// - `addr`: The address of the contract. | |||
| /// - `output`: A reference to the output data buffer to write the code hash. | |||
| /// | |||
There was a problem hiding this comment.
It would be nice to instead explain the returned value here :)
There was a problem hiding this comment.
add documentation on implementation level
There was a problem hiding this comment.
Documentation on there doesn't hurt. But why not accurately document the API? Also the error case was documented before. Now this is implied by the returned value. I don't understand why this is no longer documented.
There was a problem hiding this comment.
I can add it here as well but I think this is documented well, it takes an address and reference to write output.
There was a problem hiding this comment.
Yeah please do. We don't want people to needlessly puzzle together the full picture of our API. The error was documented before.
There was a problem hiding this comment.
Yes sure it writes output. Now the error is gone from the method signature. How are people supposed to know what output is written when the address was not a contract or doesn't exist?
Description
Update
ext_code_hashto match EIP-1052 specs. Since all possible results are written into output pointer then there's no need for a return value.paritytech/revive#77