This repository has been archived by the owner on Jul 9, 2021. It is now read-only.
[3.0] Exchange Rich Reverts (once more, with feeling!) #1761
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fix compilation issues in `exchange`.
Add rich reverts chai helper tests to `dev-utils`
Rename `StandardError` to `StringRevertError`. Rename `RichRevertAbi` to `RevertErrorAbi`. Make `RevertError` extend `Error` so it can be thrown. Add `RevertError` tests.
Tweak `RevertError` coercion in chai helper. Add more `RevertError` chai helper tests.
…and throw `RevertError`s. Remove no longer necessary dependency on `ethers.js` in `@0x/base-contract`.
…de not passing all arguments to previous handler. In `@0x/dev-utils` add more `RevertError` chai helper tests for backwards compatibility with `rejectedWith`. In `@0x/dev-utils` instead of overriding `rejectedWith`, add a new method `revertWith`. In `@0x/dev-utils` clean up the code for the `RevertError` chai helper.
…te file from `chai_setup.ts`. In `@0x/dev-utils`: Add chai support for ganache and geth transaction reverts.
|
Approved the Typescript code only. |
abandeali1
suggested changes
Apr 10, 2019
abandeali1
suggested changes
Apr 10, 2019
hysz
reviewed
Apr 10, 2019
hysz
approved these changes
Apr 10, 2019
There was a problem hiding this comment.
Looks awesome and I'm excited to use it! Good to merge after addressing @abandeali1's comments.
…ector` in `RichErrors.StandardError`
…in `MixinAssetProxyDispatcher.sol`
…eRevertErrors.SignatureError`. In `@0x/contracts-exchange`: Add `signerAddress` and `signature` to `SignatureError` reverts.
…ertErrors`. In `@0x/contracts-exchange`: Add `TransactionSignatureError`, supplanting `TransactionErrorCodes.BAD_SIGNATURE`, and associated test.
dorothy-zbornak
force-pushed
the
feature/contracts/exchange/3.0/rich-revert-reasons
branch
from
17cc83d to
cb010eb
Compare
abandeali1
reviewed
Apr 12, 2019
| // | | 68 | n | Left-aligned message bytes | | ||
|
|
||
| // Get the size of the string data/payload. | ||
| let revertDataSize := sub(returndatasize(), 36) |
There was a problem hiding this comment.
Omitting the first 36 bytes works if we assume that the revert data is always a string, but what if we move to rich revert reasons for asset proxies too? In that case, it's probably better to just include all of the return data and decode client side based off of the function selector.
There was a problem hiding this comment.
Implemented here.
abandeali1
reviewed
Apr 12, 2019
abandeali1
reviewed
Apr 12, 2019
| } | ||
| return isValid; | ||
| // Static call the verification function. | ||
| (bool didSucceed, bytes memory returnData) = walletAddress.staticcall(callData); |
There was a problem hiding this comment.
I believe that you can just do IValidator.isValidSignature(...) and it will compile with the staticcall opcode in Solidity ^0.5.0.
There was a problem hiding this comment.
Yeah, but don't we want to catch the revert?
There was a problem hiding this comment.
We can return the revert data, too.
abandeali1
reviewed
Apr 12, 2019
Co-Authored-By: dorothy-zbornak <[email protected]>
…Error` to `ExchangeRevertErrors`. Update `AssetProxyTransferError` to accept arbitrary `errorData` bytes instead of a `revertReason` string.
…eValidatorError` rich reverts. In `@0x/contracts-exchange`: Change `AssetProxyTransferError` to accept a `revertData` bytes instead of a `revertReason` string. In `@0x/contracts-exchange`: Aadd `contracts/test/TestRevertReceiver.sol` for testing that validator/wallet reverts are properly wrapped.
abandeali1
approved these changes
Apr 12, 2019
…t to `== 1` instead of `!= 0`.
This was referenced Apr 19, 2019
dorothy-zbornak
deleted the
feature/contracts/exchange/3.0/rich-revert-reasons
branch
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This is a complete overhaul and more holistic implementation of my initial proof-of-concept #1688. Kindly refer to that PR for background on the issue, encoding details, and to mock my rookie naiveté.
Summary
This PR introduces the tools for raising, consuming, and testing rich revert errors from smart contracts. It introduces a new idiom for testing contract reverts (rich or plain) via Chai extensions. It also upgrades all errors thrown by the Exchange contract into said rich reverts.
Notable Changes
Smart Contracts
contracts/utilswhich exposes arrevert()function.rich reverts.
Tooling
RevertErrorbase class in@0x/utils:StringRevertError) and rich reverts.TransactionExecutionErrors which carry an ABI-encoded payload of the original error.Errorfor backwards compatiblity with existing tests.RevertErrortypes for the Exchange in@0x/order-utils.ExchangeRevertErrors.@0x/dev-utils:equal()to andbecome()to support comparisons betweenRevertErrortypes.revertWith(), for testing both transaction and call promises that should revert. It's also backwards compatible with regular string reverts, such as those in@0x/types.RevertReason.expectContractCallFailedAsync()andexpectTransactionFailedAsync()functions from@0x/contracts-test-utilswith a nice Chai expression:return expect(tx).to.revertWith(...).@0x/contracts-test-utilsand others now inherit theirchaiSetupfrom@0x/dev-utils.chaiSetupso the new Chai extensions are available just about everywhere.@0x/base-contractto automatically decode and throwRevertErrors.Testing instructions
Affected contract tests have been updated/converted to handle rich reverts conform to the new testing idiom. Specifically, these packages are:
@0x/contracts-exchange@0x/contracts-extensionsTests for
RevertErrors and the new Chai extensions have been added to:@0x/utils@0x/dev-utilsTypes of changes
Checklist:
[WIP]if necessary.Addresses 0xProject/ZEIPs/issues/32