Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Allow decoder to use 4byte.directory to decode #6057

Merged
merged 8 commits into from Jun 15, 2023
Merged

Allow decoder to use 4byte.directory to decode #6057

merged 8 commits into from Jun 15, 2023

Conversation

haltman-at
Copy link
Contributor

@haltman-at haltman-at commented May 25, 2023

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 that decodeTransaction 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 in interpretations.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 also overrideContext 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 also isForSelectorBasedDecoding, 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, as decodeCalldata 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. But decodeCalldata 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 to decodeWithAdditionalContexts.)

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 and async-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.

@davidmurdoch
Copy link
Member

@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.

@haltman-at we're looking for sub-millisecond 4byte-to-signature lookups, so an HTTP API approach won't work for us.

Copy link
Contributor

@gnidan gnidan left a 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/decoder/lib/fetch-signatures.ts Show resolved Hide resolved
@@ -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
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@@ -1000,3 +1001,16 @@ export interface OverlongArrayOrStringStrictModeError {
export interface InternalFunctionInABIError {
kind: "InternalFunctionInABIError";
}

/**
* This doesn't really belong here, but we need something for
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, good point.

Copy link
Contributor

@gnidan gnidan left a 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)

@haltman-at haltman-at requested a review from gnidan June 9, 2023 04:05
@haltman-at
Copy link
Contributor Author

OK, I've added settings. Per discussion elsewhere, I've made it disabled by default rather than enabled by default.

Copy link
Contributor

@gnidan gnidan left a 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!

@haltman-at haltman-at merged commit 95bc2de into develop Jun 15, 2023
10 checks passed
@haltman-at haltman-at deleted the 4byte branch June 15, 2023 19:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants