moonbeam icon indicating copy to clipboard operation
moonbeam copied to clipboard

Improve precompiles revert messages + fix few issues

Open nanocryk opened this issue 2 years ago • 1 comments

What does it do?

New features :

  • New Revert type that allows to handle revert messages with support for formatting consistent across precompiles, and backtraces (here for which field in nested structs have a problem). Most precompile-utils parsing functions now return MayRevert<T>, which is an alias for Result<T, Revert>. MayRevert can be converted to an EvmResult manually or with ?, which will perform the formatting.
  • RevertReason represent common kinds of reasons a precompile can revert. It also have a Custom(String) variant for other issues.
  • When parsing input, in_field and in_array should be used on Revert, RevertReason or MayRevert to annotate which field is the cause of a revert. When parsing function arguments the macro parse_args! can be used to shortly list the arguments with their types, which will automatically add in_field with the field name in camelCase.
  • When implementing EvmData on custom structs, map_in_tuple_to_field allows to parse the struct using the tuple implementation of EvmData (which is non-trivial), and then convert tuple indices in reverts into field names of the structure.

Changes :

  • Randomness precompile currently use errors, which consume all the remaining gas and whose error message is not accessible to users. They are thus replaced with reverts.
  • Revert messages have been harmonized, mainly thanks to RevertReason. Tests are thus updated.
  • #1710 changed revert messages to Solidity-encode them, which allow them to be decoded and displayed in RPC. However since the CallPermit precompile propagate the revert of its subcall, it made the revert double encoded. This is now fixed.
  • CallPermit, in case of success, returns what is returned by the subcall. However since the signature state that the return value is bytes, Solidity contracts calling CallPermit will thus try to decode Solidity encoded bytes, while the precompile returned the subcall return data as-is. The return data is thus wrapped into a Solidity-encoded bytes. This may overlap with #1733.

What important points reviewers should know?

Is there something left for follow-up PRs?

Parsing of all arguments of our precompiles is not tested much, as it is verbose to write and there are lots of possible cases to test. A follow up PR could provide utils to list arguments with valid and invalid values and test the result, ensure the revert messages are correct, etc.

We could also improve our macro to have an Action enum with struct variants listing all the fields, and the macro would generate automatically the parsing code and the full selector, removing the need to write the function signature as a string (which is error prone). Precompile execution would thus be done in 2 phases: first parse the input to get an Action with struct variants, and then forward the content of those variants to each precompile functions. This will allow to better test or even fuzz parsing input only, and test precompile logic by passing the arguments directly to the function without interacting with parsing.

What alternative implementations were considered?

Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?

What value does it bring to the blockchain users?

Users and smart contract developpers will have better revert messages when experimenting with the precompiles, knowing more precisely what is the problem and where.

nanocryk avatar Aug 08 '22 16:08 nanocryk

I find that there are too many occurrences where we have to copy a variable identifier in a literal string, t would be nice to create a small macro to avoid that, something like that should work:

macro_rules! read_fields {
	($input:ident, [$(($field:ident, $type:ty),)*]) => {
		$(let $field: $type = $input.read().in_field(stringify!($field))?;)*
	};
}

Then we can replace this

let owner: H160 = input.read::<Address>().in_field("owner")?.into();
let spender: H160 = input.read::<Address>().in_field("spender")?.into();
let value: U256 = input.read().in_field("value")?;

by

read_fields!(input, [(owner, Address), (spender, Address), (value, U256)]);

librelois avatar Aug 11 '22 13:08 librelois