asset-swapper V3 rebase, pay protocol fees, etc. #2350

dorothy-zbornak · 2019-11-18T14:43:25Z

Description

Rebases [WIP] Upgrade asset-swapper for v3. #2272 against development.
@0x/asset-swapper now pays protocol fees (now that staking gets hooked up during migration).
@0x/asset-swapper removed dependency on @0x/contract-wrappers.
@0x/migrations now deploys Forwarder AFTER staking is attached to the Exchange.

Testing instructions

Types of changes

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.

dorothy-zbornak · 2019-11-18T15:11:38Z

packages/asset-swapper/src/quote_consumers/forwarder_swap_quote_consumer.ts

        }
+        // TODO(dorothy-zbornak): Handle signature request denied


I removed the try/catch that was here because it was doing nothing. ContractError.SignatureRequestDenied didn't actually get thrown by contract wrappers, and sendTransactionAsync() only mines the TX immediately on ganache, so you won't detect a failure on mainnet.

ContractError.SignatureRequestDenied denied is easy enough to detect, but maybe the logic should live in another package. Detecting a CompleteFillFailed will have to be accomplished by doing a callAsync() at the same block - 1 the TX was mined. Let's add tasks for this.

dorothy-zbornak · 2019-11-18T15:13:06Z

packages/asset-swapper/src/quote_consumers/exchange_swap_quote_consumer.ts

        }
+        // TODO(dorothy-zbornak): Handle signature request denied


I removed the try/catch that was here because it was doing nothing. ContractError.SignatureRequestDenied didn't actually get thrown by contract wrappers, and sendTransactionAsync() only mines the TX immediately on ganache, so you won't detect a failure on mainnet.

ContractError.SignatureRequestDenied denied is easy enough to detect, but maybe the logic should live in another package. Detecting a IncompleteFill will have to be accomplished by doing a callAsync() at the same block -1 the TX was mined. Let's add tasks for this.

Hmm, yeah that try-catch was residual from asset-buyer, I do want to ask though, not very familiar with the nuances of the wrappers, how are errors handled? One of the large feedbacks I received at EthWaterloo was the vague errors thrown, don't want us to make asset-swapper more opaque with errors.

SignatureRequestDenied was just running a regex against the thrown error message (by Metamask or w/e).

If the use awaitTransactionSuccessAsync() instead of sendTransactionAsync(), it will throw if the TX fails. Then we can use callAsync() to replay the TX and get the actual rich revert, which gives us deeper detail.

dorothy-zbornak · 2019-11-18T15:15:10Z

packages/asset-swapper/src/quote_consumers/forwarder_swap_quote_consumer.ts


+        const ethAmountWithFees = affiliateFeeUtils
+            .getTotalEthAmountWithAffiliateFee(worstCaseQuoteInfo, feePercentage)
+            .plus(worstCaseQuoteInfo.protocolFeeInEthAmount);


Not sure if this is what we want, but here I just add protocol fees to the total payable amount.

getTotalEthAmountWithAffiliateFee will consider protocol fees since affiliateFee is dependent on protocolFee (as of current forwarder design).

ahh, yeah, that makes sense

dorothy-zbornak · 2019-11-18T15:15:22Z

packages/asset-swapper/src/quote_consumers/forwarder_swap_quote_consumer.ts

+        // if no ethAmount is provided, default to the worst totalTakerAssetAmount
+        const ethAmountWithFees = affiliateFeeUtils
+            .getTotalEthAmountWithAffiliateFee(worstCaseQuoteInfo, feePercentage)
+            .plus(worstCaseQuoteInfo.protocolFeeInEthAmount);


same case, do not need to add protocolFee

dorothy-zbornak · 2019-11-18T15:19:17Z

packages/asset-swapper/src/quote_consumers/exchange_swap_quote_consumer.ts

            methodAbi,
+            ethAmount: quote.worstCaseQuoteInfo.protocolFeeInEthAmount,


Added to pay protocol fees.

nice catch, I completely forgot about adding that in.

dorothy-zbornak · 2019-11-18T15:19:25Z

packages/asset-swapper/src/quote_consumers/exchange_swap_quote_consumer.ts

-            } else {
-                throw err;
-            }
+        const value = ethAmount || quote.worstCaseQuoteInfo.protocolFeeInEthAmount;


~~Added to pay protocol fees.~~

buildsize · 2019-11-19T03:08:24Z

File name	Previous Size	New Size	Change
init.py	60.25 KB	60.25 KB	0 bytes (0%)
abi_gen_dummy.ts	77.71 KB	[deleted]
lib_dummy.ts	7.08 KB	[deleted]
test_lib_dummy.ts	10.34 KB	[deleted]
environment.pickle	1.61 MB	1.61 MB	0 bytes (0%)
index.doctree	184.11 KB	184.11 KB	0 bytes (0%)
.buildinfo	230 bytes	230 bytes	0 bytes (0%)
genindex.html	5.6 KB	5.6 KB	0 bytes (0%)
index.html	2.52 KB	2.52 KB	0 bytes (0%)
objects.inv	375 bytes	375 bytes	0 bytes (0%)
py-modindex.html	3.07 KB	3.07 KB	0 bytes (0%)
search.html	2.84 KB	2.84 KB	0 bytes (0%)
searchindex.js	5.98 KB	5.98 KB	0 bytes (0%)
index.rst.txt	415 bytes	415 bytes	0 bytes (0%)
alabaster.css	10.92 KB	10.92 KB	0 bytes (0%)
basic.css	11.89 KB	11.89 KB	0 bytes (0%)
custom.css	42 bytes	42 bytes	0 bytes (0%)
doctools.js	9.05 KB	9.05 KB	0 bytes (0%)
documentation_options.js	303 bytes	303 bytes	0 bytes (0%)
file.png	286 bytes	286 bytes	0 bytes (0%)
jquery-[version].js	273.79 KB	273.79 KB	0 bytes (0%)
jquery.js	86.08 KB	86.08 KB	0 bytes (0%)
language_data.js	10.59 KB	10.59 KB	0 bytes (0%)
minus.png	90 bytes	90 bytes	0 bytes (0%)
plus.png	90 bytes	90 bytes	0 bytes (0%)
pygments.css	4.69 KB	4.69 KB	0 bytes (0%)
searchtools.js	15.61 KB	15.61 KB	0 bytes (0%)
underscore-[version].js	34.34 KB	34.34 KB	0 bytes (0%)
underscore.js	11.86 KB	11.86 KB	0 bytes (0%)
contract_addresses.html	16.8 KB	16.8 KB	0 bytes (0%)
contract_artifacts.html	8.24 KB	8.24 KB	0 bytes (0%)
json_schemas.html	12.55 KB	12.55 KB	0 bytes (0%)
order_utils.html	46.85 KB	46.85 KB	0 bytes (0%)
erc20_token.html	93.54 KB	93.54 KB	0 bytes (0%)
exchange.html	718.37 KB	718.37 KB	0 bytes (0%)
tx_params.html	9.41 KB	9.41 KB	0 bytes (0%)
local_message_signer.html	15.07 KB	15.07 KB	0 bytes (0%)
asset_data_utils.html	22.65 KB	22.65 KB	0 bytes (0%)
default_api.html	113.16 KB	113.16 KB	0 bytes (0%)
asset_proxy_owner.html	337.37 KB	337.37 KB	0 bytes (0%)
coordinator.html	128.71 KB	128.71 KB	0 bytes (0%)
coordinator_registry.html	39.58 KB	39.58 KB	0 bytes (0%)
dutch_auction.html	66.12 KB	66.12 KB	0 bytes (0%)
erc20_proxy.html	109.06 KB	109.06 KB	0 bytes (0%)
erc721_proxy.html	109.18 KB	109.18 KB	0 bytes (0%)
erc721_token.html	150.19 KB	150.19 KB	0 bytes (0%)
forwarder.html	121.63 KB	121.63 KB	0 bytes (0%)
i_asset_proxy.html	40.17 KB	40.17 KB	0 bytes (0%)
i_validator.html	27.05 KB	27.05 KB	0 bytes (0%)
i_wallet.html	24.88 KB	24.88 KB	0 bytes (0%)
multi_asset_proxy.html	144.02 KB	144.02 KB	0 bytes (0%)
order_validator.html	107.67 KB	107.67 KB	0 bytes (0%)
weth9.html	132.08 KB	132.08 KB	0 bytes (0%)
zrx_token.html	107.6 KB	107.6 KB	0 bytes (0%)
dev_utils.html	491.09 KB	491.09 KB	0 bytes (0%)
types.html	8.54 KB	8.54 KB	0 bytes (0%)
erc1155_mintable.html	276.5 KB	276.5 KB	0 bytes (0%)
erc1155_proxy.html	130.37 KB	130.37 KB	0 bytes (0%)
static_call_proxy.html	34.02 KB	34.02 KB	0 bytes (0%)

coveralls · 2019-11-19T04:56:14Z

Coverage remained the same at 75.612% when pulling 38adc72 on fix/asset-swapper/rebase-and-pay-protocol-fees into 176e088 on development.

hysz · 2019-11-19T19:10:29Z

packages/asset-swapper/test/sorting_utils_test.ts

+const FAKE_ERC20_TAKER_ASSET_DATA = '0xf47261b22222222222222222222222222222222222222222222222222222222222222222';
+const FAKE_ERC20_MAKER_ASSET_DATA = '0xf47261b11111111111111111111111111111111111111111111111111111111111111111';
+
+describe('sortingUtils', () => {


Nice work! Very cleanly communicates / tests the sorting.

I didn't write like 99% of this PR (it just looks that way because of the rebase), so props to @dave4506

hysz · 2019-11-19T19:15:43Z

packages/asset-swapper/src/utils/order_prune_utils.ts

+    }
+
+    // tslint:disable-next-line: prefer-function-over-method
+    private _filterForFillableAndPermittedFeeTypeOrders(


There's a lot going on in this function. Could be good to add some inline comments.

hysz

Nice job, it's clear a lot of meticulous work went into this 👏👏👏. I mostly dug into the new logic. I think it looks great - very clear and thoroughly tested.

I skimmed the upgrades to existing logic and from what I can see they make sense. Although it would be good to have a engineer who's more intimately familiar with these packages to read through these areas as well.

fragosti

Lets MERGEEEEEEEEEEE

dave4506 · 2019-11-19T19:39:15Z

packages/asset-swapper/src/quote_consumers/exchange_swap_quote_consumer.ts

        }
+        // TODO(dorothy-zbornak): Handle signature request denied


Hmm, yeah that try-catch was residual from asset-buyer, I do want to ask though, not very familiar with the nuances of the wrappers, how are errors handled? One of the large feedbacks I received at EthWaterloo was the vague errors thrown, don't want us to make asset-swapper more opaque with errors.

dave4506 · 2019-11-19T19:41:23Z

packages/asset-swapper/src/quote_consumers/forwarder_swap_quote_consumer.ts


+        const ethAmountWithFees = affiliateFeeUtils
+            .getTotalEthAmountWithAffiliateFee(worstCaseQuoteInfo, feePercentage)
+            .plus(worstCaseQuoteInfo.protocolFeeInEthAmount);


getTotalEthAmountWithAffiliateFee will consider protocol fees since affiliateFee is dependent on protocolFee (as of current forwarder design).

dave4506 · 2019-11-19T19:42:46Z

packages/asset-swapper/src/quote_consumers/forwarder_swap_quote_consumer.ts

+        // if no ethAmount is provided, default to the worst totalTakerAssetAmount
+        const ethAmountWithFees = affiliateFeeUtils
+            .getTotalEthAmountWithAffiliateFee(worstCaseQuoteInfo, feePercentage)
+            .plus(worstCaseQuoteInfo.protocolFeeInEthAmount);


same case, do not need to add protocolFee

dorothy-zbornak force-pushed the fix/asset-swapper/rebase-and-pay-protocol-fees branch from c3a4b7e to a6a09ef Compare November 18, 2019 14:44

dorothy-zbornak commented Nov 18, 2019

View reviewed changes

dorothy-zbornak requested a review from dave4506 November 18, 2019 15:50

dorothy-zbornak marked this pull request as ready for review November 18, 2019 15:51

dorothy-zbornak force-pushed the fix/asset-swapper/rebase-and-pay-protocol-fees branch from a6a09ef to 7ee46f3 Compare November 19, 2019 02:56

dorothy-zbornak requested a review from fabioberger as a code owner November 19, 2019 04:44

hysz reviewed Nov 19, 2019

View reviewed changes

hysz approved these changes Nov 19, 2019

View reviewed changes

dave4506 mentioned this pull request Nov 19, 2019

[WIP] Upgrade asset-swapper for v3. #2272

Closed

4 tasks

fragosti approved these changes Nov 19, 2019

View reviewed changes

dorothy-zbornak added 4 commits November 19, 2019 14:49

@0x/migrations: Deploy Forwarder AFTER hooking up staking.

32258ef

@0x/asset-swapper: Rebase against development and pay protocol fees.

439c98a

@0x/asset-swapper: Fix static test errors.

6529b7e

@0x/monorepo-scripts: Add Set to EXTERNAL_TYPE_MAP.

362c7c5

dorothy-zbornak force-pushed the fix/asset-swapper/rebase-and-pay-protocol-fees branch from 554658c to 362c7c5 Compare November 19, 2019 19:49

dave4506 approved these changes Nov 19, 2019

View reviewed changes

@0x/asset-swapper: Remove redundant protocol fee calculation.

38adc72

dave4506 approved these changes Nov 19, 2019

View reviewed changes

dave4506 merged commit 8b94bbb into development Nov 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

asset-swapper V3 rebase, pay protocol fees, etc. #2350

asset-swapper V3 rebase, pay protocol fees, etc. #2350

dorothy-zbornak commented Nov 18, 2019

dorothy-zbornak Nov 18, 2019 •

edited

Loading

dorothy-zbornak Nov 18, 2019 •

edited

Loading

dave4506 Nov 19, 2019

dorothy-zbornak Nov 19, 2019

dorothy-zbornak Nov 18, 2019

dave4506 Nov 19, 2019

dorothy-zbornak Nov 19, 2019

dorothy-zbornak Nov 18, 2019

dave4506 Nov 19, 2019

dorothy-zbornak Nov 18, 2019

dave4506 Nov 19, 2019

dorothy-zbornak Nov 18, 2019 •

edited

Loading

buildsize bot commented Nov 19, 2019 •

edited

Loading

coveralls commented Nov 19, 2019 •

edited

Loading

hysz Nov 19, 2019

dorothy-zbornak Nov 19, 2019

hysz Nov 19, 2019

hysz left a comment

fragosti left a comment

dave4506 Nov 19, 2019

dave4506 Nov 19, 2019

dave4506 Nov 19, 2019

		methodAbi,
		ethAmount: quote.worstCaseQuoteInfo.protocolFeeInEthAmount,

asset-swapper V3 rebase, pay protocol fees, etc. #2350

asset-swapper V3 rebase, pay protocol fees, etc. #2350

Conversation

dorothy-zbornak commented Nov 18, 2019

Description

Testing instructions

Types of changes

Checklist:

dorothy-zbornak Nov 18, 2019 • edited Loading

Choose a reason for hiding this comment

dorothy-zbornak Nov 18, 2019 • edited Loading

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

dorothy-zbornak Nov 18, 2019 • edited Loading

Choose a reason for hiding this comment

buildsize bot commented Nov 19, 2019 • edited Loading

coveralls commented Nov 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hysz left a comment

Choose a reason for hiding this comment

fragosti left a 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

dorothy-zbornak Nov 18, 2019 •

edited

Loading

dorothy-zbornak Nov 18, 2019 •

edited

Loading

dorothy-zbornak Nov 18, 2019 •

edited

Loading

buildsize bot commented Nov 19, 2019 •

edited

Loading

coveralls commented Nov 19, 2019 •

edited

Loading