🔂 Liquidity Provider Asset Swapper integration #2505

PirosB3 · 2020-02-29T01:15:08Z

Description

This PR contains the implementation of "liquidity providers". This new feature allows third-party developers to plug in their own private liquidity sources that adhere to the IERC20Bridge.bridgeTransferFrom() interface.

Testing instructions

Tested using a mocked liquidity provider, unit tests, and more testing to come!

Types of changes

New feature (non-breaking change which adds functionality)

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.

…new contracts and their respective interfaces

…tation

…eature/plp-integration # Conflicts: # packages/asset-swapper/package.json

cleancoindev · 2020-02-29T06:39:29Z

I have a review: the circleci says:

https://circleci.com/gh/0xProject/0x-monorepo/110617?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

Failure due to missing or extra semicolons in various places on the first static-tests check screen of fail?

cleancoindev · 2020-02-29T06:40:44Z

and this one test-rest another review:

https://circleci.com/gh/0xProject/0x-monorepo/110620?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

You are missing a {} says the interpreter error?

coveralls · 2020-03-02T19:31:06Z

Coverage increased (+0.05%) to 79.739% when pulling 659e899 on feature/plp-integration into b8ec7f5 on development.

moodlezoup

Why are we exporting DummyLiquidityProvider and DummyLiquidityProviderRegistry in @0x/contract-wrappers and @0x/contract-artifacts? We should avoid doing so if possible

moodlezoup · 2020-03-02T23:35:42Z

contracts/erc20-bridge-sampler/contracts/src/DummyLiquidityProvider.sol

+        returns (uint256 makerTokenAmount) {
+            makerTokenAmount = sellAmount - 1;


style nit:

Suggested change

returns (uint256 makerTokenAmount) {

makerTokenAmount = sellAmount - 1;

returns (uint256 makerTokenAmount)

{

makerTokenAmount = sellAmount - 1;

moodlezoup · 2020-03-02T23:35:48Z

contracts/erc20-bridge-sampler/contracts/src/DummyLiquidityProvider.sol

+        returns (uint256 takerTokenAmount) {
+            takerTokenAmount = buyAmount + 1;


moodlezoup · 2020-03-02T23:36:11Z

contracts/erc20-bridge-sampler/contracts/src/DummyLiquidityProviderRegistry.sol

+        address takerToken,
+        address makerToken,
+        address poolAddress
+    ) external


Suggested change

) external

)

external

moodlezoup · 2020-03-02T23:36:38Z

contracts/erc20-bridge-sampler/contracts/src/DummyLiquidityProvider.sol

+    constructor()
+        public
+    // solhint-disable-next-line no-empty-blocks
+    {}


no constructor needed

moodlezoup · 2020-03-02T23:36:52Z

contracts/erc20-bridge-sampler/contracts/src/DummyLiquidityProviderRegistry.sol

+    constructor()
+        public
+    // solhint-disable-next-line no-empty-blocks
+    {}


no constructor needed

moodlezoup · 2020-03-02T23:51:57Z

packages/asset-swapper/test/dex_sampler_test.ts

+                }
+
+                expect(sellQuotes.length).to.eql(1);
+                const iquidityPoolSellQuotes: DexSample[] = sellQuotes[0];


Suggested change

const iquidityPoolSellQuotes: DexSample[] = sellQuotes[0];

const liquidityPoolSellQuotes: DexSample[] = sellQuotes[0];

moodlezoup · 2020-03-02T23:56:46Z

packages/asset-swapper/test/market_operation_utils_test.ts

@@ -141,6 +141,7 @@ describe('MarketOperationUtils tests', () => {
        makerToken: string,
        takerToken: string,
        fillAmounts: BigNumber[],
+        liquidityProviderAddress?: string | undefined,


The | undefined is redundant with the ?, no?

fixed! thanks

moodlezoup · 2020-03-02T23:57:07Z

packages/asset-swapper/test/market_operation_utils_test.ts

@@ -172,14 +195,47 @@ describe('MarketOperationUtils tests', () => {
        makerToken: string,
        takerToken: string,
        fillAmounts: BigNumber[],
+        liquidityProviderAddress?: string | undefined,


fixed! thanks

moodlezoup · 2020-03-03T00:00:35Z

packages/asset-swapper/test/market_operation_utils_test.ts

+                const yAsset = randomAddress();
+                const toSell = Web3Wrapper.toBaseUnitAmount(10, 18);
+
+                const [getSellQuiotesParams, getSellQuotesFn] = callTradeOperationAndRetainLiquidityProviderParams(


Suggested change

const [getSellQuiotesParams, getSellQuotesFn] = callTradeOperationAndRetainLiquidityProviderParams(

const [getSellQuotesParams, getSellQuotesFn] = callTradeOperationAndRetainLiquidityProviderParams(

moodlezoup · 2020-03-03T00:03:19Z

packages/asset-swapper/test/market_operation_utils_test.ts

+                    getLiquidityProviderFn,
+                ] = getLiquidityProviderFromRegistryAndReturnCallParameters(liquidityPoolAddress);
+                replaceSamplerOps({
+                    getOrderFillableTakerAmounts: () => [new BigNumber(0)],


Suggested change

getOrderFillableTakerAmounts: () => [new BigNumber(0)],

getOrderFillableTakerAmounts: () => [constants.ZERO_AMOUNT],

hysz · 2020-03-03T18:58:41Z

packages/asset-swapper/src/utils/market_operation_utils/index.ts

@@ -130,6 +146,7 @@ export class MarketOperationUtils {
            makerToken,
            collapsePath(optimalPath, false),
            _opts.bridgeSlippage,
+            liquidityPoolAddress,


Nit - liquidityPool -> liquidityProvider

hysz · 2020-03-03T18:59:46Z

packages/asset-swapper/src/utils/market_operation_utils/index.ts

    private _createBuyOrdersPathFromSamplerResultIfExists(
        nativeOrders: SignedOrder[],
        makerAmount: BigNumber,
        nativeOrderFillableAmounts: BigNumber[],
        dexQuotes: DexSample[][],
        ethToTakerAssetRate: BigNumber,
        opts: GetMarketOrdersOpts,
+        liquidityPoolAddress?: string | undefined,


Nit - liquidityPool -> liquidityProvider

packages/asset-swapper/src/utils/market_operation_utils/sampler_operations.ts

hysz · 2020-03-03T19:04:25Z

contracts/erc20-bridge-sampler/contracts/src/DummyLiquidityProvider.sol

+    ///      selling `sellAmount` of `takerToken`.
+    /// @param sellAmount Amount of `takerToken` to sell.
+    /// @return makerTokenAmount Amount of `makerToken` that would be obtained.
+    function getSellQuote(


Verified that this matches the spec ✅

hysz · 2020-03-03T19:04:32Z

contracts/erc20-bridge-sampler/contracts/src/DummyLiquidityProvider.sol

+    ///      order to obtain `buyAmount` of `makerToken`.
+    /// @param buyAmount Amount of `makerToken` to buy.
+    /// @return takerTokenAmount Amount of `takerToken` that would need to be sold.
+    function getBuyQuote(


Verified that this matches the spec ✅

hysz · 2020-03-03T19:06:52Z

contracts/erc20-bridge-sampler/contracts/src/DummyLiquidityProviderRegistry.sol

+    /// @param makerToken Second asset managed by pool.
+    /// @param poolAddress Address of pool.
+    function setLiquidityProviderForMarket(
+        address takerToken,


Nit - this naming may be confusing because either asset could be the taker/maker token on a given trade. Something like xToken / yToken or firstToken / secondToken may be better 🤷‍♂.

hysz

LGTM! Verified the Solidity against the spec and it looks good. It could be good for an engineer more familiar with Asset Swapper to give this a review as well.

…eature/plp-integration # Conflicts: # packages/asset-swapper/package.json # packages/asset-swapper/src/utils/market_operation_utils/sampler.ts # packages/asset-swapper/src/utils/market_operation_utils/types.ts # packages/asset-swapper/test/market_operation_utils_test.ts

hysz · 2020-03-05T18:14:50Z

contracts/erc20-bridge-sampler/contracts/src/DummyLiquidityProvider.sol

+        view
+        returns (uint256 makerTokenAmount)
+    {
+        makerTokenAmount = sellAmount - 1;


Could be good to use LibSafeMath here and below to avoid wonky output in future testing.

dorothy-zbornak

Good job wrangling this nightmare-level codebase! Just some (small?) requests.

contracts/utils/contracts/src/DeploymentConstants.sol

dorothy-zbornak · 2020-03-05T17:17:40Z

packages/contract-artifacts/artifacts/ERC20BridgeSampler.json

@@ -0,0 +1,346 @@
+{


We never used to expose this contract before. Why now?

exposing this contract allows me to deploy it on-chain (Ganache) and perform E2E simulations of sampling PLP liquidity

dorothy-zbornak · 2020-03-05T17:39:37Z

packages/asset-swapper/src/utils/market_operation_utils/sampler_operations.ts

+        takerToken: string,
+        makerToken: string,


Can we flip the order of these? Just to be consistent with the other ops. I know it's the other way in the contract.

dorothy-zbornak · 2020-03-05T17:39:46Z

packages/asset-swapper/src/utils/market_operation_utils/sampler_operations.ts

+        takerToken: string,
+        makerToken: string,


dorothy-zbornak · 2020-03-05T17:39:59Z

packages/asset-swapper/src/utils/market_operation_utils/sampler_operations.ts

+        takerToken: string,
+        makerToken: string,


dorothy-zbornak · 2020-03-05T18:06:13Z

packages/asset-swapper/test/dex_sampler_test.ts

@@ -326,6 +341,104 @@ describe('DexSampler tests', () => {
            );
            expect(quotes).to.deep.eq(expectedQuotes);
        });
+
+        describe('LiquidityProvider Operations', () => {


I don't know how I feel about these tests. These seem be partially testing the test contracts. I'd also rather we didn't do on-chain tests in this package.

Do you want to unit test the DexOrderSampler ops? If so, extending the MockSamplerContract is the way to go, like how the other tests in this file do it. This would also remove the need for the new test contracts.

But if you want to unit test ERC20BridgeSampler integration with liquidity providers, that should be done in the contracts/erc20-bridge-sampler package.

that's a wise point of view, I'm going to move my tests in erc20-bridge-sampler test package

I refactored and moved those unit tests to erc20-bridge-sampler. I also added extra unit tests using MockSamplerContract

dorothy-zbornak · 2020-03-05T18:10:07Z

packages/asset-swapper/test/market_operation_utils_test.ts

+                const liquidityProviderAddress = randomAddress();
+                const xAsset = randomAddress();
+                const yAsset = randomAddress();
+                const toSell = Web3Wrapper.toBaseUnitAmount(10, 18);


Nit: There's a better version of this in @0x/utils (fromTokenUnitAmount(), toTokenUnitAmount()).

Thanks! I didn't know about this utility :)

dorothy-zbornak · 2020-03-05T18:43:02Z

packages/asset-swapper/test/market_operation_utils_test.ts

+                const toSell = Web3Wrapper.toBaseUnitAmount(10, 18);
+
+                const [getSellQuotesParams, getSellQuotesFn] = callTradeOperationAndRetainLiquidityProviderParams(
+                    createGetMultipleSellQuotesOperationFromRates,


Feels like this whole test case should be with the "sell" test cases.

good catch, my bad. Fixing

dorothy-zbornak · 2020-03-05T18:46:05Z

packages/asset-swapper/test/market_operation_utils_test.ts

+                    ORDER_DOMAIN,
+                    registryAddress,
+                );
+                const result = await sampler.getMarketSellOrdersAsync(


Yeah, definitely.

good catch, my bad. Fixing

…eature/plp-integration # Conflicts: # packages/contract-addresses/addresses.json # packages/contract-wrappers/package.json

dorothy-zbornak

Easy changes then all good! Fantastic job!

dorothy-zbornak · 2020-03-06T18:22:02Z

packages/contract-wrappers/package.json

@@ -31,7 +31,7 @@
        "wrappers:generate": "abi-gen --abis  ${npm_package_config_abis} --output src/generated-wrappers --backend ethers"
    },
    "config": {
-        "abis": "../contract-artifacts/artifacts/@(DevUtils|ERC20Token|ERC721Token|Exchange|Forwarder|IAssetData|LibTransactionDecoder|WETH9|Coordinator|Staking|StakingProxy|IERC20BridgeSampler|ERC20BridgeSampler|GodsUnchainedValidator|Broker|ILiquidityProvider|ILiquidityProviderRegistry|DummyLiquidityProvider|DummyLiquidityProviderRegistry).json"
+        "abis": "../contract-artifacts/artifacts/@(DevUtils|ERC20Token|ERC721Token|Exchange|Forwarder|IAssetData|LibTransactionDecoder|WETH9|Coordinator|Staking|StakingProxy|IERC20BridgeSampler|ERC20BridgeSampler|GodsUnchainedValidator|Broker|ILiquidityProvider|ILiquidityProviderRegistry|DummyLiquidityProvider|DummyLiquidityProviderRegistry|MaximumGasPrice).json"


Can we make sure there are no Dummy* contracts exported by this package? No one would use them outside of tests. If they want them, they can import them directly from the erc20-bridge-sampler package.

dorothy-zbornak · 2020-03-06T18:24:45Z

contracts/erc20-bridge-sampler/test/erc20-bridge-sampler.ts

@@ -716,6 +718,93 @@ blockchainTests('erc20-bridge-sampler', env => {
        });
    });

+    blockchainTests.resets('getLiquidityProviderFromRegistry', () => {


Nit: All the test cases here are constant, so no chain state to reset. This can just be a regular describe() block.

dorothy-zbornak · 2020-03-06T18:29:06Z

contracts/erc20-bridge-sampler/test/erc20-bridge-sampler.ts

+            await testContract.createTokenExchanges([MAKER_TOKEN, TAKER_TOKEN]).awaitTransactionSuccessAsync()
+                .txHashPromise;


No need to call createTokenExchanges. It just creates uniswap exchanges, which you aren't using.

dorothy-zbornak · 2020-03-06T18:30:28Z

contracts/erc20-bridge-sampler/test/erc20-bridge-sampler.ts

+            );
+            await registryContract
+                .setLiquidityProviderForMarket(xAsset, yAsset, liquidityProvider.address)
+                .awaitTransactionSuccessAsync().txHashPromise;


Suggested change

.awaitTransactionSuccessAsync().txHashPromise;

.awaitTransactionSuccessAsync();

dorothy-zbornak · 2020-03-06T18:34:43Z

contracts/erc20-bridge-sampler/test/erc20-bridge-sampler.ts

@@ -10,6 +10,8 @@ import { Order } from '@0x/types';
 import { BigNumber, hexUtils } from '@0x/utils';
 import * as _ from 'lodash';

+import { DummyLiquidityProviderContract, DummyLiquidityProviderRegistryContract } from '../src';


You can pull these in from './wrappers'.

Daniel Pyrathon added 12 commits February 27, 2020 12:52

created DummyPLPRegistry and DummyPLP + generated wrappers for these …

77d7afe

…new contracts and their respective interfaces

generated wrappers for ERC20 sampler

3c0fd54

initial unit tests for DEX sampler

8be60e2

added more unit tests for PLP DEX sampling

9608d8f

added more unit tests, reinforced existing tests, added more implemen…

7181be8

…tation

tidy up tests, add other tests

e5df51a

refactor tests, and add more tests

bcc3e5e

Added linting and prettifying

9d2aef5

Merge branch 'development' of github.com:0xProject/0x-monorepo into f…

5d4bbd5

…eature/plp-integration # Conflicts: # packages/asset-swapper/package.json

re-generate ERC20BridgeSampler artifacts

08619e2

performed a rename

7495ac8

Update artifacts after rename

fa8e8ad

PirosB3 requested review from abandeali1, BMillman19, dave4506, fragosti and hysz as code owners February 29, 2020 01:15

Daniel Pyrathon added 6 commits March 2, 2020 10:01

transformed the new artifacts

82b0f85

refactored sampler operations into a single external file

82de5ad

factored out interfaces in types.ts

599af2b

fixed a bug with imports

8186d62

added linting to the contracts

b0fd78d

refactored imports

18ce19a

moodlezoup suggested changes Mar 3, 2020

View reviewed changes

hysz reviewed Mar 3, 2020

View reviewed changes

packages/asset-swapper/src/utils/market_operation_utils/sampler_operations.ts Show resolved Hide resolved

hysz reviewed Mar 3, 2020

View reviewed changes

hysz approved these changes Mar 3, 2020

View reviewed changes

Daniel Pyrathon added 7 commits March 3, 2020 14:02

update sampler address

49b7c9c

addressed PR comments

807904b

refactor more code

99dc4b8

refresh wrappers and artifacts

bc7504c

adds possibility to exclude PLP

3eb1429

upgrades contracts-erc20-bridge-sampler dependency

d0d7d27

hysz reviewed Mar 5, 2020

View reviewed changes

dorothy-zbornak suggested changes Mar 5, 2020

View reviewed changes

Daniel Pyrathon added 7 commits March 5, 2020 16:07

invert maker and taker token variable

61f03b0

added unit tests to avoid regression due to variable order

32e1ae2

prettify and lint

fa886aa

moved unit tests to the appropriate sections

dbc5c0d

added unit tests for the Liquidity Provider

36c457f

remove unit tests that were ported to erc20-bridge-sampler package

17b2320

Merge branch 'development' of github.com:0xProject/0x-monorepo into f…

d849813

…eature/plp-integration # Conflicts: # packages/contract-addresses/addresses.json # packages/contract-wrappers/package.json

dorothy-zbornak approved these changes Mar 6, 2020

View reviewed changes

Completed feedback

659e899

PirosB3 merged commit 68207aa into development Mar 6, 2020

PirosB3 deleted the feature/plp-integration branch March 6, 2020 20:34

PirosB3 mentioned this pull request Mar 6, 2020

Update CHANGELOG.json #2515

Merged

		returns (uint256 makerTokenAmount) {
		makerTokenAmount = sellAmount - 1;

		returns (uint256 takerTokenAmount) {
		takerTokenAmount = buyAmount + 1;

	const iquidityPoolSellQuotes: DexSample[] = sellQuotes[0];
	const liquidityPoolSellQuotes: DexSample[] = sellQuotes[0];

	const [getSellQuiotesParams, getSellQuotesFn] = callTradeOperationAndRetainLiquidityProviderParams(
	const [getSellQuotesParams, getSellQuotesFn] = callTradeOperationAndRetainLiquidityProviderParams(

	getOrderFillableTakerAmounts: () => [new BigNumber(0)],
	getOrderFillableTakerAmounts: () => [constants.ZERO_AMOUNT],

		await testContract.createTokenExchanges([MAKER_TOKEN, TAKER_TOKEN]).awaitTransactionSuccessAsync()
		.txHashPromise;

	.awaitTransactionSuccessAsync().txHashPromise;
	.awaitTransactionSuccessAsync();

🔂 Liquidity Provider Asset Swapper integration #2505

🔂 Liquidity Provider Asset Swapper integration #2505

Conversation

PirosB3 commented Feb 29, 2020

Description

Testing instructions

Types of changes

Checklist:

cleancoindev commented Feb 29, 2020

cleancoindev commented Feb 29, 2020

coveralls commented Mar 2, 2020 • edited Loading

moodlezoup 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dorothy-zbornak 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

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

dorothy-zbornak left a comment

Choose a reason for hiding this comment

dorothy-zbornak Mar 6, 2020 • 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

coveralls commented Mar 2, 2020 •

edited

Loading

dorothy-zbornak Mar 6, 2020 •

edited

Loading