Integration of EIP1962 precompile #10874
Conversation
|
It looks like @shamatar signed our Contributor License Agreement. 👍 Many thanks, Parity Technologies CLA Bot |
jam10o-new
added
F8-enhancement 🎊
| @@ -548,14 +572,39 @@ impl Bn128Pairing { | |||
| } | |||
| } | |||
|
|
|||
| impl Implementation for EIP1962 { | |||
| /// Can fail in many cases, so leave it for an implementation | |||
There was a problem hiding this comment.
No need for a doc-comment here. Also: I'm not sure what "…so leave it for an implementation" means, can you clarify?
There was a problem hiding this comment.
Parity client implements gas metering for BN128 and Modexp itself. In case of EIP 1962 gas estimating function is implemented as a part of the crate, so this implementation just calls such implementation. For a doc-comment - sure, I'll add one.
| impl Implementation for EIP1962 { | ||
| /// Can fail in many cases, so leave it for an implementation | ||
| fn execute(&self, input: &[u8], output: &mut BytesRef) -> Result<(), &'static str> { | ||
| let result = eth_pairings::public_interface::API::run(&input); |
There was a problem hiding this comment.
eth_pairings::public_interface::API is an odd sequence of names, why not just eth_pairings::run? I mean, if it's callable from the outside it's part of the api and part of the public interface so it seems tautological to repeat "api" and "public interface".
| fn execute(&self, input: &[u8], output: &mut BytesRef) -> Result<(), &'static str> { | ||
| let result = eth_pairings::public_interface::API::run(&input); | ||
| if result.is_err() { | ||
| trace!("EIP1962 error: {:?}", result.err().expect("is error")); |
There was a problem hiding this comment.
Better to use error! here.
| let result = eth_pairings::public_interface::API::run(&input); | ||
| if result.is_err() { | ||
| trace!("EIP1962 error: {:?}", result.err().expect("is error")); | ||
| return Err("Precompile call was unsuccessful"); |
There was a problem hiding this comment.
Can you include the actual error in the string?
There was a problem hiding this comment.
Sure, I was not sure if it would be handy
| trace!("EIP1962 error: {:?}", result.err().expect("is error")); | ||
| return Err("Precompile call was unsuccessful"); | ||
| } | ||
| let result: Vec<u8> = result.expect("some result"); |
There was a problem hiding this comment.
All expects should motivate the reason why it is safe to unwrap, e.g. "we just checked if is_err so we know it's ok; qed"
| return Err("Precompile call returned no data"); | ||
| } | ||
| if result.len() == 1 { | ||
| // manually made BE of Uint256(1) |
There was a problem hiding this comment.
| // manually made BE of Uint256(1) | |
| // manually make BE of Uint256(1) |
| buf[31] = result[0]; | ||
| output.write(0, &buf); | ||
| } else { | ||
| output.write(0, &result[..]); |
There was a problem hiding this comment.
Is this guaranteed to be of a certain length (32 bytes?)? If so, can you do &result[..32] here?
There was a problem hiding this comment.
A single byte is returned iff a pairing operation is performed, so I manually put this byte into last byte of 32 bytes buffer to simulate serialization of U256. In case of other operations returned length is not equal to one, so I write directly to the output.
| assert_eq!(f.cost(&input[..]), expected_cost.into()); | ||
| } | ||
|
|
||
| // test for a valid return value for some BLS12 test vector |
There was a problem hiding this comment.
Can you add the source for this test vector to the comment?
| impl Implementation for EIP1962 { | ||
| /// Can fail in many cases, so leave it for an implementation | ||
| fn execute(&self, input: &[u8], output: &mut BytesRef) -> Result<(), &'static str> { | ||
| let result = eth_pairings::public_interface::API::run(&input); |
There was a problem hiding this comment.
| let result = eth_pairings::public_interface::API::run(&input); | |
| let result = eth_pairings::public_interface::API::run(&input).map_err(|e| ...insert anything here you want to display e...)?; |
This PR incorporates reference implementation for EIP1962 into Parity client.
Due to the structure of EIP functionality this integration delegates most of the work to the implementation:
U256::max_value()is returned