[WIP] Rich Revert Reasons PoC (Exchange only for now) #1688

merklejerk · 2019-03-09T01:21:39Z

Description

This is a proof-of-concept migration from regular string-based revert reasons to much more informative, ABI-encoded revert reasons. This is based around discussions of EIP 838.

NOTE I've only migrated the 0.4.x version of the exchange contracts because ATM other contract packages in development use a conflicting 0.5.x version of Solidity. Once PR #1682 for modernizing all the legacy contracts gets merged, I can start migrating en masse. But before then, we should have a discussion about this approach, because the implementation has some pain points and touches several packages.

Some Background

Current Solidity revert strings are already returned as calldata ABI-encoded bytes with a method signature of Error(string), but you can actually return any arbitrary bytes with an assembly call to revert().

Revert data can only be reliably recovered during an eth_call, because transactions only produce a hash and receipt, with a single status bit indicating success or failure. But a significant issue we've run into is just detecting a revert during an eth_call, particularly with Geth. A Geth node will simply return the revert bytes, with no signal that the call reverted. Parity supposedly returns a signal, but, as we don't currently test against Parity, I cannot personally confirm this. The current hack (already implemented in /packages/base-contract is to basically check if the returned data starts with the bytes4 selector of the standard Error signature.

Where We Are Now

There have been discussions around various schemes of returning rich revert data. The EVM does not seem to sanitize what gets returned via revert(), so many options are on the table. Ultimately, for this PoC, we've settled on simply ABI-encoding against custom method signatures for specific Exchange errors. One common example would be OrderStatusError(string,uint8), which encodes an order's hash and current status. This is thrown whenever the order is in an incompatible state during an operation.

While this approach is relatively simple and wire-compact, it does require prior knowledge of all revert signatures to semi-confidently discern which return values are true reverts and which are just arbitrary bytes. It is also not backwards compatible-- clients that only look for Error(string) signatures will likely either fail at the decoding step or miss these reverts entirely.

Significant additions have been made to the order-utils, test-utils, and abi-gen-templates packages to help intercept, decode, construct, and test against these new revert formats. Small modifications were also made to other sundry packages so they could anticipate the new reverts and pass their tests.

For the Solidity implementation, please take a look at /contracts/exchange/.../MRichErrors.sol. Usage is peppered among the various Exchange contracts (everywhere there used to be a require() or revert()).

The Typescript implementation currently resides at `/packages/order-utils/.../rich_reverts.ts. Some tests also exist to demonstrate usage (though more could be used).

There is also a chai extension implemented in contract-wrappers that demonstrates integration with chai. This will probably be moved to test-utils in future commits because that makes more sense.

Testing instructions

Just run the regular monorepo test suite since this PR spans many packages.

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

Prefix PR title with [WIP] if necessary.
Add tests to cover changes as needed.
Update documentation as needed.
Add new entries to the relevant CHANGELOG.jsons.

…et@^1.0.26. Update order-watcher's websocket dependency from ^1.0.25 (broken on some node architectures) to ^1.0.26.

…et@^1.0.25.

Update order-utils changelog and manifest. Revert metacoin to development (again).

…cs generation.

Modify contract method template to attach returnData field to decoding errors.

abandeali1 · 2019-03-11T18:29:59Z

contracts/exchange/contracts/src/MixinAssetProxyDispatcher.sol

-            currentAssetProxy == address(0),
-            "ASSET_PROXY_ALREADY_EXISTS"
-        );
+        if (currentAssetProxy != address(0))


Let's use brackets after all if statements please.

abandeali1 · 2019-03-11T18:34:27Z

contracts/exchange/contracts/src/MixinAssetProxyDispatcher.sol

-            );
-
+            if (assetProxy == address(0))
+                rrevert(AssetProxyDispatchError(AssetProxyDispatchErrorCodes.UNKNOWN_ASSET_PROXY));


I wonder if both of these errors AssetProxyDispatcher errors should just rethrow the entire assetData. Thoughts @fabioberger ?

Yeah, I'd be in favor. Imagine you have an order that fails with the error. If we include the assetData that triggered these failures, it'll be easier to debug.

abandeali1 · 2019-03-11T19:46:25Z

contracts/exchange/contracts/src/MixinExchangeCore.sol

+                rrevert(
+                    SignatureError(
+                        orderInfo.orderHash,
+                        SignatureErrorCodes.BAD_SIGNATURE


Hmm, also wondering if the signature errors should be more specific. In general, it feels like we need to make some assumptions about what information the end user actually has.

I actually think this is fine. If logic inside the isValidSignature reverts, they will get a detailed error message for those specific issues. This one comes to down to ecrecover returning a different address, it not being presigned or the wallet contract returning false. Which one of those occured can be deduced by looking at the signature type passed in.

abandeali1 · 2019-03-11T19:46:58Z

contracts/exchange/contracts/src/MixinExchangeCore.sol

-        );
-
+        if (takerAssetFillAmount == 0)
+            rrevert(FillError(orderInfo.orderHash, FillErrorCodes.INVALID_TAKER_AMOUNT));


Why use rrevert over rrequire?

Because it would look like rrequire(takerAssetFillAmount != 0, FillError(...)). It's a nicer experience, but it means the revert reason has to be ABI encoded before the condition is checked, prematurely wasting a lot of gas.

That answers my prev question. Makes sense

Solidity could potentially fix that on the language level later.

What about smth like condition || rrevert(Error(stuff))? Will it be lazy-evaluated?

Maybe. Will check on remix.

Yup, condition || rrevert(Error(stuff)); works. So that's an option if people want a more compact alternative.

abandeali1 · 2019-03-11T19:52:03Z

contracts/exchange/contracts/src/mixins/MRichErrors.sol

+
+
+// This should probably be moved to contracts-exchange-lib
+contract MRichErrors


Some thoughts on naming here:

Contracts with the M prefix in the mixins directory don't contain actual implementations, just the function signatures (this is so that we can easily mock out the functions in tests, which we don't actually really do yet). The actual implementation of the contract would be prefixed with Mixin.

Since we'll have these in different packages going forward, maybe the name should be more specific to avoid naming collisions (like MixinExchangeRichErrors).

abandeali1 · 2019-03-11T20:13:42Z

contracts/exchange/contracts/src/mixins/MRichErrors.sol

+        pure
+    {
+        assembly {
+            if iszero(and(success, 0x00000000000000000000000000000000000000000000000000000000000000FF)) {


I don't think we actually need this mask?

This is my concern: https://solidity.readthedocs.io/en/v0.5.5/security-considerations.html#minor-details

I tried it out in remix and function arguments don't get bitmasked before hitting the body, so the dirty higher bits passed into rrequire() persist.

abandeali1 · 2019-03-11T20:15:05Z

contracts/exchange/contracts/src/mixins/MRichErrors.sol

+    /// may be preferable (gas-wise) to rrequire()
+    /// since you can avoid the needless abi encoding if the test passes.
+    /// @param errorData ABI encoded error data.
+    function rrevert(bytes memory errorData)


rrevert and rrequire should probably be in a separate contract in utils, since these will be reused in other packages.

Definitely. Will move them once all contracts agree on solidity version.

abandeali1 · 2019-03-11T20:18:37Z

contracts/exchange/package.json

@@ -1,6 +1,6 @@
 {
    "name": "@0x/contracts-exchange",
-    "version": "1.0.9",
+    "version": "1.1.0",


You don't actually need to bump versions in package.json (this happens automatically when we publish the packages). It shouldn't matter either way.

abandeali1 · 2019-03-11T21:52:31Z

packages/order-utils/src/rich_reverts.ts

+/**
+ * Base type for rich revert reasons.
+ */
+export abstract class RichRevertReason {


Does it make sense for this to be in the order-utils package? contract-wrappers feels more natural to me since we're decoding our contact's reverts specifically.

hysz · 2019-03-11T22:21:42Z

it does require prior knowledge of all revert signatures to semi-confidently discern which return values are true reverts and which are just arbitrary bytes

I wonder if it would make sense to use a generic signature, like ZeroExError(bytes), where the input bytes is a nested encoding of the error message. Now we have a single 4-byte selector to infer an error state.

Example:

errorMessage = abiEncode(OrderStatusError(string,uint8))`
zeroExErrorMessage = abiEncode(ZeroExError(errorMessage))

If this could be beneficial, it's worth noting that it shouldn't be too much additional cost. We just prepend the selector, a generic offset (0x..20) and the length of errorMessage (which we should already know).

Edit:
This could also make it easier for contracts to detect errors when calling NoThrow functions which simply copy relay the returndata.

merklejerk · 2019-03-12T03:47:35Z

@hysz I like this. Much less chance of collisions with the selector bytes.

LogvinovLeon · 2019-03-12T15:44:31Z

contracts/exchange/CHANGELOG.json

+                "note": "Add rich revert reasons"
+            }
+        ]
+    },


You should append those changes to the previous entry. It doesn't have the timestamp which means - it's not published. Just change the version.

Also add the PR number please 🙏

LogvinovLeon · 2019-03-12T15:45:55Z

contracts/exchange/contracts/src/MixinAssetProxyDispatcher.sol

-            "ASSET_PROXY_ALREADY_EXISTS"
-        );
+        if (currentAssetProxy != address(0))
+            rrevert(AssetProxyExistsError(currentAssetProxy));


I like the require pattern. I think we should have smth like rrequire

LogvinovLeon · 2019-03-12T15:48:05Z

contracts/exchange/contracts/src/MixinExchangeCore.sol

-        );
-
+        if (takerAssetFillAmount == 0)
+            rrevert(FillError(orderInfo.orderHash, FillErrorCodes.INVALID_TAKER_AMOUNT));


That answers my prev question. Makes sense

LogvinovLeon · 2019-03-12T15:48:37Z

contracts/exchange/contracts/src/MixinExchangeCore.sol

-        );
-
+        if (takerAssetFillAmount == 0)
+            rrevert(FillError(orderInfo.orderHash, FillErrorCodes.INVALID_TAKER_AMOUNT));


Solidity could potentially fix that on the language level later.

LogvinovLeon · 2019-03-12T15:49:58Z

contracts/exchange/contracts/src/MixinExchangeCore.sol

-        );
-
+        if (takerAssetFillAmount == 0)
+            rrevert(FillError(orderInfo.orderHash, FillErrorCodes.INVALID_TAKER_AMOUNT));


What about smth like condition || rrevert(Error(stuff))? Will it be lazy-evaluated?

LogvinovLeon · 2019-03-12T16:09:42Z

packages/order-utils/src/rich_reverts.ts

+    RICH_REVERT_REGISTRY,
+);
+
+const DECODER_CACHE: ObjectMap<Decoder> = _.zipObject(


Our naming convention for Objects is stuffToStuff1ToStuff2 etc...
For example selectorToDecoder

LogvinovLeon · 2019-03-12T16:10:34Z

packages/order-utils/src/rich_reverts.ts

+    _.map(RICH_REVERT_REGISTRY, r => createDecoder(r.abi)),
+);
+
+function checkArgEquality(type: string, a: ArgTypes, b: ArgTypes): boolean {


Nit: Maybe name those params lhs and rhs?left hand side

LogvinovLeon · 2019-03-12T16:11:57Z

packages/order-utils/src/rich_reverts.ts

+    };
+}
+
+function declarationToAbi(decl: string): RichRevertAbi {


Ethers.js should have a function to parse the ABI string. Check my prev PR

LogvinovLeon · 2019-03-12T16:13:27Z

packages/order-utils/test/rich_reverts_test.ts

+const assert = chai.assert;
+
+// tslint:disable:custom-no-magic-numbers
+describe.only('RichRevertReasons', () => {


Suggested change

describe.only('RichRevertReasons', () => {

describe('RichRevertReasons', () => {

LogvinovLeon · 2019-03-12T16:14:00Z

packages/types/CHANGELOG.json

+                "note": "Move OrderStatus enum from /contracts/test-utils into here"
+            }
+        ]
+    },


Merge with prev entry

And PR # :)

fabioberger · 2019-03-14T18:48:40Z

contracts/exchange/CHANGELOG.json

+                "note": "Add rich revert reasons"
+            }
+        ]
+    },


Also add the PR number please 🙏

fabioberger · 2019-03-14T18:51:25Z

contracts/exchange/contracts/src/MixinAssetProxyDispatcher.sol

-            );
-
+            if (assetProxy == address(0))
+                rrevert(AssetProxyDispatchError(AssetProxyDispatchErrorCodes.UNKNOWN_ASSET_PROXY));


Yeah, I'd be in favor. Imagine you have an order that fails with the error. If we include the assetData that triggered these failures, it'll be easier to debug.

fabioberger · 2019-03-14T19:03:12Z

contracts/exchange/contracts/src/MixinExchangeCore.sol

+                rrevert(
+                    SignatureError(
+                        orderInfo.orderHash,
+                        SignatureErrorCodes.BAD_SIGNATURE


I actually think this is fine. If logic inside the isValidSignature reverts, they will get a detailed error message for those specific issues. This one comes to down to ecrecover returning a different address, it not being presigned or the wallet contract returning false. Which one of those occured can be deduced by looking at the signature type passed in.

fabioberger · 2019-03-14T19:04:43Z

contracts/exchange/contracts/src/MixinExchangeCore.sol

-        );
-
+        if (takerAssetFilledAmount > takerAssetFillAmount)
+            rrevert(FillError(orderInfo.orderHash, FillErrorCodes.TAKER_OVERPAY));


Let's def. be using curly brackets for all conditionals.

fabioberger · 2019-03-14T22:08:36Z

contracts/exchange/contracts/src/MixinExchangeCore.sol

+        if (safeMul(makerAssetFilledAmount, order.takerAssetAmount)
+                >
+                safeMul(order.makerAssetAmount, takerAssetFilledAmount)) {
+            rrevert(FillError(orderInfo.orderHash, FillErrorCodes.INVALID_FILL_PRICE));


Technically we'd need takerAssetFilledAmount and makerAssetFilledAmount here since without those values, one couldn't figure out what went wrong. However, this is a cautionary assertion that should never hit, so maybe we can pass on this one.

fabioberger · 2019-03-14T22:39:45Z

packages/order-utils/package.json

@@ -1,6 +1,6 @@
 {
    "name": "@0x/order-utils",
-    "version": "7.0.2",
+    "version": "7.1.0",


Revert. You get the drill ;)

fabioberger · 2019-03-14T22:40:33Z

packages/order-utils/src/index.ts

@@ -72,3 +72,5 @@ export {
    FeeOrdersAndRemainingFeeAmount,
    OrdersAndRemainingFillAmount,
 } from './types';
+
+export * from './rich_reverts';


I'd rather we explicitly list the items we want exported. Someone might add an export to that file and be surprise to find that it's been auto-magically exported by the package.

fabioberger · 2019-03-14T22:43:13Z

packages/order-utils/src/rich_reverts.ts

+    new IncompleteFillError(),
+];
+
+const RICH_REVERT_LUT: ObjectMap<RichRevertReason> = _.zipObject(


Please use readable variable names over conciseness.

fabioberger · 2019-03-14T22:44:38Z

packages/order-utils/test/rich_reverts_test.ts

+
+    describe('decoding', () => {
+        interface DecoderTestData {
+            cls: { name: string };


fabioberger · 2019-03-14T22:45:21Z

packages/types/CHANGELOG.json

+                "note": "Move OrderStatus enum from /contracts/test-utils into here"
+            }
+        ]
+    },


And PR # :)

merklejerk added 11 commits March 8, 2019 01:26

Remove websocket@1.0.25 entry from yarn.lock so yarn installs websock…

483c77f

…et@^1.0.26. Update order-watcher's websocket dependency from ^1.0.25 (broken on some node architectures) to ^1.0.26.

Add PR

755c4da

Update connect package as well because it was also asking for websock…

644f54b

…et@^1.0.25.

Switch exchange contract and related tools to use rich reverts.

2368140

Fix RichRevertReason matching against empty fields.

d02e1e9

Add RichRevertReason tests.

8374deb

Undo test fiddling with order-watcher.

ec27453

Update order-utils changelog and manifest. Revert metacoin to development (again).

Export ethereum-types/RichRevertAbi in packages that need them for do…

9111b7d

…cs generation.

Fix broken RichRevertAbi import/export in 0x package.

f68bf26

Fix broken eth_call test cases against geth.

1d63a3b

Modify contract method template to attach returnData field to decoding errors.

Run through prettier and linter

b1127c5

merklejerk requested review from abandeali1, fabioberger, fragosti, hysz and LogvinovLeon as code owners March 9, 2019 01:21

probot-autolabeler bot added 0x.js abi-gen-templates @0x/connect contracts labels Mar 9, 2019

abandeali1 suggested changes Mar 11, 2019

View reviewed changes

LogvinovLeon suggested changes Mar 12, 2019

View reviewed changes

fabioberger suggested changes Mar 14, 2019

View reviewed changes

dorothy-zbornak closed this Mar 29, 2019

dorothy-zbornak mentioned this pull request Apr 5, 2019

[3.0] Exchange Rich Reverts (once more, with feeling!) #1761

Merged

4 tasks

xianny added @0x/abi-gen-templates and removed abi-gen-templates labels Jun 26, 2019



		// This should probably be moved to contracts-exchange-lib
		contract MRichErrors

	describe.only('RichRevertReasons', () => {
	describe('RichRevertReasons', () => {

[WIP] Rich Revert Reasons PoC (Exchange only for now) #1688

[WIP] Rich Revert Reasons PoC (Exchange only for now) #1688

Conversation

merklejerk commented Mar 9, 2019

Description

Some Background

Where We Are Now

Testing instructions

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hysz commented Mar 11, 2019 • edited Loading

merklejerk commented Mar 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hysz commented Mar 11, 2019 •

edited

Loading