Skip to content
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

Fix tuple encoding in precompiles return position #2068

Merged
merged 3 commits into from Feb 8, 2023

Conversation

nanocryk
Copy link
Contributor

@nanocryk nanocryk commented Jan 27, 2023

What does it do?

In Solidity tuple encoding depends on if the content has static size or not. Dynamic size tuples are encoded with an offset to the tuple data, while static size tuples the fields are encoded inlined.

Function returning multiple values look like they are returning tuples, however they are always encoded inline regardless of if the size is dynamic or not. Note that while structs are usually encoded like tuples, using a struct as return will be encoded differently if the struct is dynamic sized or not.

This PR makes the following changes:

  • Add a new function encode_as_function_return_value that performs the correct encoding. It relies on a new function in EvmData trait called is_explicit_tuple that should only return true for tuples (it has a default implementation to false so that implementing the trait on other types don't require to think about it). In the case that the data being provided to encode_as_function_return_value returns true for is_explicit_tuple and false to has_static_size, the first 32 bytes (the offset) is stripped.
  • This issue was noticed with a function of the Referenda precompile, which returns a dynamic-sized tuple. To keep the same ABI, its return type is changed to an equivalent struct.

What important points reviewers should know?

Is there something left for follow-up PRs?

  • EvmData derive macro

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?

@nanocryk nanocryk added A0-pleasereview Pull request needs code review. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. not-breaking Does not need to be mentioned in breaking changes labels Jan 27, 2023
min_support: UnboundedBytes,
}

impl EvmData for TrackInfo {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be done as a derive macro?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can add an EvmData derive macro in a follow up PR yes :)

@crystalin crystalin added D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited and removed D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Feb 7, 2023
@crystalin crystalin merged commit e0f31c8 into master Feb 8, 2023
@crystalin crystalin deleted the jeremy-precompile-macro-returns-tuple branch February 8, 2023 09:03
@notlesh notlesh added D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Feb 14, 2023
@crystalin crystalin changed the title Fix tuple encoding in return position Fix tuple encoding in precompiles return position Feb 22, 2023
imstar15 pushed a commit to OAK-Foundation/moonbeam that referenced this pull request May 16, 2023
…#2068)

* fix returns tuple encoding

* use struct in Referanda precompile to keep same encoding

* fix typos
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A0-pleasereview Pull request needs code review. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited not-breaking Does not need to be mentioned in breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants