-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Allow decoder to use 4byte.directory to decode #6057
Conversation
@haltman-at we're looking for sub-millisecond 4byte-to-signature lookups, so an HTTP API approach won't work for us. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks like it will work, but I'm still wrestling with how we can make this less of a hack. In the meantime, have some requested changes for minor stuff!
packages/codec/lib/core.ts
Outdated
@@ -51,7 +51,8 @@ export function* decodeVariable( | |||
*/ | |||
export function* decodeCalldata( | |||
info: Evm.EvmInfo, | |||
isConstructor?: boolean //ignored if context! trust context instead if have | |||
isConstructor?: boolean, //ignored if context! trust context instead if have | |||
strictAbiMode?: boolean //used for selector-based decoding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to document somewhere what this option does, since it kind of turns this function into something of a chimera. Like, how is it used for selector-based decoding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. I mean all of this stuff doesn't really have documentation, as this is codec rather than decoder. But I can expand the comment to explain it better.
packages/codec/lib/format/errors.ts
Outdated
@@ -1000,3 +1001,16 @@ export interface OverlongArrayOrStringStrictModeError { | |||
export interface InternalFunctionInABIError { | |||
kind: "InternalFunctionInABIError"; | |||
} | |||
|
|||
/** | |||
* This doesn't really belong here, but we need something for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe better to say "This isn't so much a Format error", rather than "here", since the docstring might not show up in the context you expect always
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, good point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more thing then I think we can merge this and figure out how to clean up the code flow later...
Namely, we should allow disabling this functionality (but leave it on by default)
OK, I've added settings. Per discussion elsewhere, I've made it disabled by default rather than enabled by default. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to go. Thanks @haltman-at!
Addresses #6029. Has a "logical merge conflict" with #6050 that I'll have to account for, but nothing that should be a problem to deal with.
This PR allows the decoder to use 4byte.directory to decode transactions it otherwise can't. Now, due to the return format of
decodeTransaction()
, it wasn't reasonable to simply have these selector-based decodings appear as the return value. The problem is thatdecodeTransaction
returns a single decoding, not an array of decodings like the other decoding functions; and since there could be more than one decoding resulting from 4byte.directory, you see the problem.Instead, we're handling this with
interpretations
. If the decoding has type"message"
or"unknown"
, we now do an extra step where we attempt to decode with 4byte.directory. Any valid function decodings that come back will go ininterpretations.selectorBasedDecodings
, which is an array (although it's only included if it's nonempty, since interpretations aren't included when not relevant).To do this, first we fetch relevant signatures from 4byte.directory. I've put this functionality in its own file in
decoder
, but I kind of wonder whether it should go in its own package. I doubt it at this point, which is why I didn't, but worth bringing up, I think. Also, I know that @davidmurdoch intends to put some sort of 4byte.directory functionality in Ganache, so we might want to discuss if these should be combined in any way.(Note: I handled pagination by incrementing our own page counter, rather than making use of the
next
field. It seemed simpler. I doubt this will cause any problems.)Having gotten the signatures, we parse them to yield ABI entry objects. Then, we attempt to decode with these.
Now, the decoder isn't really set up to decode based on an ABI object like that, so this involves some hackery to set things up. I'll skip going over the details, because they're kind of hairy and I'm not sure there's any good way to explain them; you can go read the code. There are a few notes I want to make, however.
You'll notice I've added some extra parameters to
decodeWithAdditionalContexts
(whose name is perhaps getting a bit less appropriate). There's now alsooverrideContext
to say, hey, use this particular context, don't attempt to determine it yourself (you'll note this parameter has also been added to a bunch of things that it calls). There's alsoisForSelectorBasedDecoding
, which has a few effects.The first effect is that decoding with that flag set means selector-based decodings are not added; this is to prevent infinite loops. The second effect is that it turns on strict mode when making the call into codec to decode the calldata.
Now you may say, but
decodeCalldata
doesn't have a strict mode! Well, now it does, now there's a flag for strict mode. Again, this is a somewhat awkward addition, asdecodeCalldata
was written with the intent that it would run in nonstrict mode. Still, now, if this flag is set, then the decoding happens in strict mode, and also there's a re-encoding check at the end, which throws an error if it fails. I had to invent a new error kind for this; decoding events or returndata don't throw if the re-encoding test fails, they just don't include that decoding and continue on. ButdecodeCalldata
was made to return a single result, not multiple, so it doesn't have that option... so it throws.(This also means that
decodeBySelector
has to put a try/catch around the call todecodeWithAdditionalContexts
.)The other thing I want to note is that I'm not entirely happy with how I set up the fake context; it's likely overly complicated. In my initial tests, I found that the case where the contract wasn't recognized worked fine, but the case where it was recognized didn't. I eventually fixed that, but I have to wonder whether it would have been better to not have this case split, and to just treat all contracts as if they weren't recognized, rather than taking this split approach.
Regardless, it works! So yay.
Oh, one other thing -- to perform the requests, I did add
axios
andasync-retry
as dependencies. I'm sure MetaMask will complain about that. But, these are the same dependencies we're using for making requests in fetch-and-compile, so, uh, hopefully it's OK...? Well, we'll figure something out...Testing instructions
I've added three new tests to CI to test this; I relied on those. However, in addition to simply running the tests as-is (since they rely on mocks, and we want to make sure to test this against the real thing), I also commented out the mocking and ran the tests in that form. They still passed, so the PR appears to work also when running against the real 4byte.directory.
Warning: I didn't really test the case where pagination is relevant.