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 seth sig #818

Merged
merged 9 commits into from
Nov 13, 2021
Merged

Fix seth sig #818

merged 9 commits into from
Nov 13, 2021

Conversation

d-xo
Copy link
Contributor

@d-xo d-xo commented Sep 23, 2021

Description

  • automatically pad input for bytesX types in hevm abiencode
  • use correct syntax for string / bytesX types in seth sig
  • fuzz seth sig against 4byte.directory

Fixes #817

Checklist

  • tested locally
  • added automated tests
  • updated the docs
  • updated the changelog

@mds1
Copy link
Contributor

mds1 commented Sep 23, 2021

Ah good catch with the 4byte fuzzing to handle string types too. Everything seems to work as expected 👌

One UX comment is that it'd be nice to be able to do seth calldata "foo(string)()" "abc" instead of seth calldata "foo(string)()" '"abc"' (i.e. remove the need for double quotes), but that's bit tangential to this PR anyway

src/seth/libexec/seth/seth-sig Outdated Show resolved Hide resolved
@d-xo
Copy link
Contributor Author

d-xo commented Oct 7, 2021

looks like we also need to handle tuples... 😬

https://github.com/dapphub/dapptools/runs/3830742936#step:5:1252

@d-xo
Copy link
Contributor Author

d-xo commented Oct 7, 2021

Running into the same issue with tuples as here: ethers.js expects tuple types to be prefixed with tuple.

Not sure how to proceed here. @mds1 what was the motivation for changing the implementation of seth sig again? Just to handle uint vs uint256, or were there other concerns? This is getting quite complex and I'm considering reverting back to the old implementation....

@mds1
Copy link
Contributor

mds1 commented Oct 7, 2021

The original motiviation for changing seth sig can be found in this discussion. Basically, the old version was just an alias for the first 4 bytes of seth keccak and as a result required you to parse a function sig yourself

If we do revert to that implementation, we probably should just document that you need to pass it the already-normalized function signature, and it wont do the normalization for you.

Regarding the tuple issue, I mentioned this in the issue you linked, but upgrading to ethers v5 should resolve that (though I'm not sure offhand how many breaking changes that may introduce, but I recall it looked pretty manageable last time I checked)

@d-xo
Copy link
Contributor Author

d-xo commented Oct 7, 2021

Even if we update to a new ethers, I think we will have to extend the abi parser in hevm to support tuple types (here). I'm hitting this with seth calldata and tuple types:

$ seth calldata 'createIncentive(tuple (address,address,uint256,uint256,address) hi,uint256)' '(0x0, 0x0, 1, 1, 0x0)' 1
hevm: tuple types not supported
CallStack (from HasCallStack):
  error, called at src/EVM/ABI.hs:511:34 in hevm-0.48.1-KdjyJ1kXYK7IQeiBwTCoFo:EVM.ABI

@mds1
Copy link
Contributor

mds1 commented Oct 7, 2021

Ah, interesting! As a user I would definitely prefer if hevm supported tuples, as that seems like a pretty important feature. Especially now that ABI Encoder V2 is enabled by default so it's simpler/safer to use tuple inputs than it was with older solc versions

@d-xo
Copy link
Contributor Author

d-xo commented Nov 9, 2021

I think I'm going to merge this as is, as it already fixes quite a few issues with the current set sig implementation, and then we can tackle tuples in a seperate pr since it seems to be a bit of a bigger job. Do you agree @mds1?

@mds1
Copy link
Contributor

mds1 commented Nov 9, 2021

@d-xo Yep that seems reasonable to me!

@d-xo d-xo merged commit 5ede556 into master Nov 13, 2021
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.

seth won't generate a function signature with bytes<n> as an argument
2 participants