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

feature: contract revert trait #2182

Merged
merged 10 commits into from
Feb 28, 2023
Merged

feature: contract revert trait #2182

merged 10 commits into from
Feb 28, 2023

Conversation

prestwich
Copy link
Collaborator

@prestwich prestwich commented Feb 24, 2023

Motivation

Followup work to #2172. Allows easier decoding of error types for generated contracts

Solution

  • Introduce the ContractRevert trait
  • Auto-derive ContractRevert on the error enum in abigen! contract modules
  • Add RevertString(String) to contract error enum in abigen! contract modules
  • Add ContractError::decode_contract_revert<Err: ContractRevert(&self) -> Option<Err>
  • (drive-by) add missing FromIterator impls to ethers::core::types::Bytes

This is a breaing change to abigen! generated bindings (again) but i promise this one is also really good :)

PR Checklist

  • Added Tests
  • Added Documentation
  • Updated the changelog
  • Breaking changes

@prestwich prestwich requested a review from gakonst February 24, 2023 03:30
@prestwich prestwich self-assigned this Feb 24, 2023
@prestwich
Copy link
Collaborator Author

will address these comments tomorrow. ledger issue has worn my brain out

@prestwich prestwich force-pushed the prestwich/contract-revert branch from 0fc3a69 to 41b3d10 Compare February 25, 2023 16:10
Comment on lines +113 to +120
if let Ok(decoded) = <::std::string::String as #ethers_core::abi::AbiDecode>::decode(data) {
return Ok(Self::RevertString(decoded))
}
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't this be below the if let Ok in the macro below? So that we only decode as string as a fallback vs prioritizing it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This implementation has never been safe for users to access directly, as it does not include selector information, and ABI encoded types are incredibly ambiguous. It has always thrown bad false positives.

My belief is that strings will be more common than other error types, even for contracts that do not revert, so putting a string in the first attempt results in the fewest decode attempts

Comment on lines +13 to +27
pub trait ContractRevert: AbiDecode + AbiEncode + Send + Sync {
/// Decode the error from EVM revert data including an Error selector
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this only derived for enums?
because this clashes with EthError

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, this trait is only for unifying multiple potential ethereum errors into 1 type. Basically this should be thought of as "the errors associated with a specific contract's ABI".

I thought about breaking out a super-trait that had the shared methods on it, and decided that it was too complex for users

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i also thought about modifying AbiDecode to include decode_with_selector(selector: Selector, bytes: &[u8]) but didn't want to get into lower level changes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you can also consult the docstring, btw

/// A trait for enums unifying [`EthError`] types.
///
/// We do not recommend manual implementations of this trait. Instead, use the
/// automatically generated implementation in the [`crate::abigen`] macro```

ethers-contract/src/error.rs Outdated Show resolved Hide resolved
@prestwich prestwich marked this pull request as ready for review February 27, 2023 23:01
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.

good w/ me pending any more matt feedback

@prestwich prestwich force-pushed the prestwich/contract-revert branch from d7d3bb0 to 03e8f1f Compare February 27, 2023 23:07
@prestwich
Copy link
Collaborator Author

this is now based on latest master 👍

@gakonst
Copy link
Owner

gakonst commented Feb 27, 2023

@prestwich seems to have broken in the Seaport codegen


---- abigen::can_generate_seaport_1_0 stdout ----
error: test failed, to rerun pass `-p ethers-contract --test it`
thread 'abigen::can_generate_seaport_1_0' panicked at 'checked by len: TryFromSliceError(())', /home/runner/work/ethers-rs/ethers-rs/ethers-contract/src/error.rs:32:40
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@prestwich
Copy link
Collaborator Author

@gakonst seaport is fixed now 👍

@prestwich prestwich force-pushed the prestwich/contract-revert branch from 4c648b5 to 1fc4cd9 Compare February 28, 2023 02:02
@gakonst gakonst merged commit ded611e into master Feb 28, 2023
@gakonst gakonst deleted the prestwich/contract-revert branch February 28, 2023 07:59
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.

4 participants