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

Fix ContractRevert::decode_with_selector #2637

Merged
merged 5 commits into from
Oct 17, 2023

Conversation

aakoshh
Copy link
Contributor

@aakoshh aakoshh commented Oct 16, 2023

Motivation

I tried decoding an error returned by one of our contracts by calling ContractRevert::decode_with_selector using the enum that Abigen generates for the errors that the contract can revert with, but it kept turning into None. During debugging I noticed that while the selector is correctly accepted, it is not made available to the actual decode functions which try each possible error type, so they all fail trying to decode empty data.

Solution

Unless the error data prefix is the one corresponding to the String type, pass the data to the generated AbiDecode::decode as-is, so that it can be inspected by the independent decoders of the types that make up the enum.

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

lgtm

Comment on lines +38 to +45
if selector == String::selector() {
<Self as AbiDecode>::decode(&data[4..]).ok()
} else {
// Try with and without a prefix, just in case.
<Self as AbiDecode>::decode(data)
.or_else(|_| <Self as AbiDecode>::decode(&data[4..]))
.ok()
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see, for the string case, we expect that Self is String,
if not we try without prefix and with

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the String decoder doesn't expect the prefix. I thought everything else did, but some example tests failed without the fallback approach. I don't know why, the tests just timed out waiting for Anvil to start, twice - could be unrelated.

@mattsse mattsse merged commit 8601005 into gakonst:master Oct 17, 2023
19 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants