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(codec)!: ambiguity unit8[] and bytes #613

Merged
merged 11 commits into from
Nov 24, 2021

Conversation

mattsse
Copy link
Collaborator

@mattsse mattsse commented Nov 23, 2021

Motivation

  • There was an ambiguity for how to tokenized Vec<u8> since it was used for bytes and unit8[] alike.

Solution

  • solidity bytes is ethers::types::Bytes and
  • solidity unit8[] is Vec<u8>

This is a breaking change but can we fixed easily with into() conventions

PR Checklist

  • Added Tests
  • Added Documentation
  • Updated the changelog

@mattsse mattsse changed the title fix: unit8 encoding fix: unit8[] encoding Nov 23, 2021
@@ -117,7 +117,7 @@ macro_rules! impl_abi_type {
}

impl_abi_type!(
Vec<u8> => Bytes,
Vec<u8> => Array(Box::new(ParamType::Uint(8))),
Copy link
Owner

Choose a reason for hiding this comment

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

I think this won't work, because bytes memory which translates to Vec<u8> will now be translate to Array(Uint(8)) as a uint8[].

Could we make bytes memory translate to ethers::core::Bytes?

(I think that's probably the bug here, given the tests' output)

@mattsse mattsse changed the title fix: unit8[] encoding fix(codec)!: ambiguity unit8[] and bytes Nov 24, 2021
@gakonst gakonst merged commit 2c30468 into gakonst:master Nov 24, 2021
Copy link
Owner

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants