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

BTC Curves #2633

Merged
merged 8 commits into from
Jul 23, 2020
Merged

BTC Curves #2633

merged 8 commits into from
Jul 23, 2020

Conversation

dorothy-zbornak
Copy link
Contributor

@dorothy-zbornak dorothy-zbornak commented Jul 14, 2020

Description

This adds support for the BTC curves: 0x93054188d876f558f4a66b2ef1d97d16edf0895b and 0x7fc77b5c7614e1533320ea6ddc2eb61fa00a9714. Things to note:

  • These curves do not have an underlying asset, so we need to use get_dy() and exchange() directly. Previously we always used get_dy_underlying() and exchange_underlying().
    • This means CurveBridge and ERC20BridgeSampler had to be updated to allow passing in which functions get called (by selector).
  • These curves do not have a get_dx() function, so buy support must be approximated similar to how we do for Kyber. This motivated a big refactor of the sampler.
  • We need to store extra info per curve, on top of its pool address & tokens, in asset-swapper. So now CurveFillData has a CurveInfo object. This impacts the code in 0x-API that generates gas/fee schedules.

TODO:

  • Redeploy CurveBridge.

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 dorothy-zbornak changed the base branch from development to fix/sampler/kyber-katalyst July 14, 2020 20:33
@dekz dekz force-pushed the fix/sampler/kyber-katalyst branch 2 times, most recently from f8b7572 to 895ab73 Compare July 15, 2020 03:56
Base automatically changed from fix/sampler/kyber-katalyst to development July 15, 2020 04:05
returns (uint256[] memory takerTokenAmounts)
{
if (curveInfo.buyQuoteFunctionSelector == bytes4(0)) {
// Buys not supported on this curve, so approximate it.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No native buy quoting on the BTC curves.

{
struct CurveBridgeData {
address curveAddress;
bytes4 exchangeFunctionSelector;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTC curves just use exchange() instead of exchange_underlying().

{
struct CurveBridgeData {
address curveAddress;
bytes4 exchangeFunctionSelector;
address fromTokenAddress;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Easier than figuring out whether to call underlying_coins() or just coins().

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

saves an SLOAD too

// '0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48',
// '0xdac17f958d2ee523a2206206994597c13d831ec7',
// ],
// },
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying to use these in CurveBridge integration tests and they kept reverting when going USDC->DAI no matter what I did. Looking at etherscan they look pretty dead?

@@ -34,10 +34,6 @@ blockchainTests('erc20-bridge-sampler', env => {
const MAKER_TOKEN = randomAddress();
const TAKER_TOKEN = randomAddress();
let devUtilsAddress: string;
const FAKE_BUY_OPTS = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We no longer pass these opts into the sampler contract. They're hardcoded instead.

@dorothy-zbornak dorothy-zbornak force-pushed the feat/btc-curves branch 4 times, most recently from 9d999f2 to 72f4ab1 Compare July 21, 2020 16:46
'0x0000000000085d4780b73119b644ae5ecd22b376',
],
},
// Looks like it's dying.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ya I'd be fine with culling the old Curves with low liquidity now

fromTokenIdx: number;
toTokenIdx: number;
curve: CurveInfo;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will impact FeeSchedule generation code in API.

// exchangeFunctionSelector: CurveFunctionSelectors.exchange_underlying,
// sellQuoteFunctionSelector: CurveFunctionSelectors.get_dy_underlying,
// buyQuoteFunctionSelector: CurveFunctionSelectors.get_dx_underlying,
// poolAddress: '0xa2b47e3d5c44877cca798226b7b8118f9bfb7a56',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, seems like some activity today.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mm I think this one might stick around tbh, it's the only Compound based Curve iirc. They ebb and flow with the DeFi tides.

600k here still?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Got the tests working again (I guess forking doesn't play nice with resets) so apparently these bridges do still work.

@dorothy-zbornak dorothy-zbornak changed the title [WIP] BTC Curves BTC Curves Jul 21, 2020
@dorothy-zbornak dorothy-zbornak marked this pull request as ready for review July 21, 2020 17:47
@dorothy-zbornak dorothy-zbornak requested review from dekz and xianny July 21, 2020 17:48
Copy link
Contributor

@moodlezoup moodlezoup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

THANK YOU for this refactor holy shit the sampler was out of control

{
struct CurveBridgeData {
address curveAddress;
bytes4 exchangeFunctionSelector;
address fromTokenAddress;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

saves an SLOAD too

'0x0000000000085d4780b73119b644ae5ecd22b376',
],
},
// Looks like it's dying.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ya I'd be fine with culling the old Curves with low liquidity now

…ied curves.

`@0x/contracts-erc20-bridge-sampler`: Refactor.
`@0x/contracts-erc20-bridge-sampler`: Add support for more varied curves.
`@0x/contracts-integrations`: Update curve bridge tests.
Copy link
Member

@dekz dekz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're an absolute beast! What a piece of art this is!

High level the changes look great. Shall we deploy the Bridge and give it a run through the simbot?

// exchangeFunctionSelector: CurveFunctionSelectors.exchange_underlying,
// sellQuoteFunctionSelector: CurveFunctionSelectors.get_dy_underlying,
// buyQuoteFunctionSelector: CurveFunctionSelectors.get_dx_underlying,
// poolAddress: '0xa2b47e3d5c44877cca798226b7b8118f9bfb7a56',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mm I think this one might stick around tbh, it's the only Compound based Curve iirc. They ebb and flow with the DeFi tides.

600k here still?

@dorothy-zbornak
Copy link
Contributor Author

You're an absolute beast! What a piece of art this is!

High level the changes look great. Shall we deploy the Bridge and give it a run through the simbot?

All my sims have already been using this bridge contract.

@dorothy-zbornak dorothy-zbornak merged commit 82774df into development Jul 23, 2020
@dorothy-zbornak dorothy-zbornak deleted the feat/btc-curves branch July 23, 2020 20:43
@dekz dekz mentioned this pull request Jul 24, 2020
3 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants