Skip to content
This repository has been archived by the owner on Jul 9, 2021. It is now read-only.

[contract-wrappers] Test revert with reason when decoding call result #1090

Merged
merged 1 commit into from
Sep 28, 2018

Conversation

dekz
Copy link
Member

@dekz dekz commented Sep 26, 2018

We use in-process ganache which throws on an RPC error. This means all of our tests get a nice revert error thrown when testing against ganache. This is not possible with RPC providers and a revert with reason result is returned. Our callAsync doesn't handle this and attempts to decode the revert with reason error log as a successful log, which results in an error while decoding. Resulting in a invalid data for function output error, usually on takerFeePaid which is confusing to debug.

Description

This only works with our fork of ethers ethers-io/ethers.js#188 and will need to be re-worked when updating to Ethers.js 4.

We don't use the decode method directly, but the Interface parse function. This decodes but catches and wraps the error

This is a little awkward to test as we use vmErrorsOnRPCResponse = true by default so this would always pass unless vmErrorsOnRPCResponse = false. Created a PR to update DefinitelyTyped to remove the any.

@LogvinovLeon's PR ended up merging in checking for revert reasons, this PR now just ensures we have a test against this.

Testing instructions

Needed to create a ganache provider which has vmErrorsOnRPCResponse = false and thus the contracts need to be redeployed in this in-memory provider.

Types of changes

Checklist:

  • Prefix PR title with [WIP] if necessary.
  • Prefix PR title with bracketed package name(s) corresponding to the changed package(s). For example: [sol-cov] Fixed bug.
  • Add tests to cover changes as needed.
  • Update documentation as needed.
  • Add new entries to the relevant CHANGELOG.jsons.

@dekz dekz force-pushed the bug/contract-wrappers/handle-revert-with-reason branch 7 times, most recently from 50e4097 to 76971c6 Compare September 26, 2018 12:21
@dekz dekz changed the title [wip] [contract-wrappers] Handle revert with reason when decoding call result [contract-wrappers] [base-contract] Handle revert with reason when decoding call result Sep 26, 2018
@dekz dekz force-pushed the bug/contract-wrappers/handle-revert-with-reason branch from 76971c6 to 5624602 Compare September 26, 2018 12:45
@LogvinovLeon
Copy link
Contributor

Please rebase off development. #1069 just got merged.

@dekz dekz force-pushed the bug/contract-wrappers/handle-revert-with-reason branch 2 times, most recently from 29bf8c3 to 3696821 Compare September 27, 2018 11:49
@dekz dekz changed the title [contract-wrappers] [base-contract] Handle revert with reason when decoding call result [contract-wrappers] Test revert with reason when decoding call result Sep 27, 2018
@fabioberger
Copy link
Contributor

Ideally we would use a different provider for non-contracts tests that has vmErrorsOnRPCResponse set to false. @dekz is looking into this approach.

@coveralls
Copy link

coveralls commented Sep 27, 2018

Coverage Status

Coverage increased (+0.2%) to 85.294% when pulling 21f6072 on bug/contract-wrappers/handle-revert-with-reason into 94baded on development.

We use in-process ganache which throws on an RPC error. This means all of our tests get a nice revert error thrown when testing against ganache. This is not possible with RPC providers and a revert with reason result is returned. Our callAsync doesn't handle this and attempts to decode the revert with reason error log as a successful log, which results in an error while decoding.

This only works with our fork of ethers ethers-io/ethers.js#188 and will need to be re-worked when updating to Ethers.js 4
@dekz dekz force-pushed the bug/contract-wrappers/handle-revert-with-reason branch from 3696821 to 21f6072 Compare September 28, 2018 00:00
@dekz dekz merged commit a737cfa into development Sep 28, 2018
@dekz dekz deleted the bug/contract-wrappers/handle-revert-with-reason branch September 28, 2018 00:21
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.

5 participants