Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Split up BridgeAdapter by chain [TKR-402] #487

Merged
merged 5 commits into from
May 25, 2022
Merged

Split up BridgeAdapter by chain [TKR-402] #487

merged 5 commits into from
May 25, 2022

Conversation

moodlezoup
Copy link
Contributor

@moodlezoup moodlezoup commented May 24, 2022 •
edited
Loading

Description

Splits the BridgeAdapter contract up into one contract per chain, in order to buy us some more time wrt contract size.

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.

moodlezoup
moodlezoup commented May 24, 2022
View reviewed changes
packages/asset-swapper/src/utils/market_operation_utils/constants.ts
@@ -288,7 +288,6 @@ export const BUY_SOURCE_FILTER_BY_CHAIN_ID = valueByChainId<SourceFilters>(
ERC20BridgeSource.JetSwap,
ERC20BridgeSource.ACryptos,
ERC20BridgeSource.KyberDmm,
ERC20BridgeSource.Synapse,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

duplicate

@moodlezoup moodlezoup force-pushed the refactor/split-up-bridge-adapter branch 2 times, most recently from 397dd8b to a380bdb Compare May 24, 2022 21:26
@moodlezoup moodlezoup marked this pull request as ready for review May 25, 2022 00:07
@moodlezoup moodlezoup force-pushed the refactor/split-up-bridge-adapter branch from a380bdb to 2f06734 Compare May 25, 2022 00:09
@moodlezoup moodlezoup requested review from dekz, dextracker and kyu-c May 25, 2022 00:10
@moodlezoup moodlezoup changed the title Split up BridgeAdapter by chain Split up BridgeAdapter by chain [TKR-402] May 25, 2022
@dextracker
Copy link
Contributor

Nice one, looks good to me. quick question, this will make it so we have to load the right bridge adapter by chainId in AS? Or will pointing to the right fqt address be enough?

kyu-c
kyu-c approved these changes May 25, 2022
View reviewed changes
Copy link
Contributor

@kyu-c kyu-c left a comment

Choose a reason for hiding this comment

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

LGTM

contracts/zero-ex/contracts/src/transformers/bridges/AvalancheBridgeAdapter.sol
returns (uint256 boughtAmount)
{
uint128 protocolId = uint128(uint256(order.source) >> 128);
if (protocolId == BridgeProtocols.CURVE) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we missing AAVEV2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch

@moodlezoup
Copy link
Contributor Author

Nice one, looks good to me. quick question, this will make it so we have to load the right bridge adapter by chainId in AS? Or will pointing to the right fqt address be enough?

No the chainId check is so that we don't accidentally deploy the wrong bridge adapter for a particular chain. As far as AS is concerned, having the right FQT address is enough

@moodlezoup moodlezoup force-pushed the refactor/split-up-bridge-adapter branch from 2f06734 to 9daf21a Compare May 25, 2022 01:38
dekz
dekz approved these changes May 25, 2022
View reviewed changes
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.

Nice. Have any thoughts on a nice way to query what it supports? So we can prevent a foot gun where we enable in AS but it's not deployed on chain?

Could error on startup rather than discover in Simbot/prod

@moodlezoup moodlezoup force-pushed the refactor/split-up-bridge-adapter branch from 874805f to e655683 Compare May 25, 2022 16:47
@moodlezoup moodlezoup merged commit cf8fc0f into development May 25, 2022
@moodlezoup moodlezoup deleted the refactor/split-up-bridge-adapter branch May 25, 2022 18:18
dextracker pushed a commit that referenced this pull request Jun 15, 2022
@moodlezoup
Split up BridgeAdapter by chain [TKR-402] (#487)
68bc397 
* Split up BridgeAdapter by chain

* Fix BridgeProtocols enum

* Add isSupportedSource to bridge adapter contracts

* Add bridge adapter tests

* Fix FQT test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dekz dekz dekz approved these changes

@kyu-c kyu-c kyu-c approved these changes

@dextracker dextracker dextracker approved these changes

@abandeali1 abandeali1 Awaiting requested review from abandeali1

@dorothy-zbornak dorothy-zbornak Awaiting requested review from dorothy-zbornak

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@moodlezoup @dextracker @dekz @kyu-c