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

Expose contract revert errors in the ContractError struct #2172

Merged
merged 11 commits into from
Feb 22, 2023

Conversation

prestwich
Copy link
Collaborator

@prestwich prestwich commented Feb 21, 2023

Motivation

Users don't have access to revert data. That sucks. I'm fixing it.

Solution

  • Add logic to spelunk for json rpc revert data, inspired by ethers-rs
  • Add logic to contract
  • Break old usages (and direct instantiation) of middleware error and provider error variants of contract error enum

TODO:

  • test this code

PR Checklist

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

@prestwich prestwich added the enhancement New feature or request label Feb 21, 2023
@prestwich prestwich self-assigned this Feb 21, 2023
}

/// Convert a Middleware Error to a `ContractError`
pub fn from_middleware_error(e: M::Error) -> Self {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

can't make From<M::Error> because it potentially conflicts with blanket From<T> for T in cases where M::Error and ContractError<M> are the same type

yes this is weird

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, this would be fixed with specialization, but alas

#[error("{0}")]
MiddlewareError(M::Error),
#[error("{e}")]
MiddlewareError { e: M::Error },
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

deliberately broken 3rd party direct instantiation, so that users must use the from_middleware_error method

similar for provider error. broke it to force From usage

/// Inspired by ethers-js logic:
/// https://github.com/ethers-io/ethers.js/blob/9f990c57f0486728902d4b8e049536f2bb3487ee/packages/providers/src.ts/json-rpc-provider.ts#L25-L53
pub fn as_revert_data(&self) -> Option<Bytes> {
self.is_revert().then(|| self.data.as_ref().and_then(spelunk_revert)).flatten()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TIL about bool::then

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.

feature 👍, breaking change good w/ me

@prestwich
Copy link
Collaborator Author

looks like linter error is related to #2171

@prestwich prestwich marked this pull request as ready for review February 22, 2023 01:36
@prestwich
Copy link
Collaborator Author

cc @DaniPopes this touches your recent work 😅

Copy link
Collaborator

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

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

@prestwich That's fine since you're integrating it with the general Contract error, my PR was mostly about having a better API for the multicall call methods

ethers-contract/tests/it/contract.rs Outdated Show resolved Hide resolved
@gakonst gakonst merged commit 2090bf5 into master Feb 22, 2023
@gakonst gakonst deleted the prestwich/spelunk branch February 22, 2023 22:52
@prestwich prestwich mentioned this pull request Feb 24, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants