Skip to content
This repository has been archived by the owner on Apr 28, 2022. It is now read-only.

Add balancer vault artifact #630

Merged
merged 4 commits into from
May 25, 2021
Merged

Add balancer vault artifact #630

merged 4 commits into from
May 25, 2021

Conversation

nlordell
Copy link
Contributor

@nlordell nlordell commented May 20, 2021

This PR refactors how the artifact vendoring works in order to add new Balancer Vault contract. In particular this PR:

  • Strips even more JSON artifact properties that aren't used, namely:
    • contractName: the build script was overriding the contract name for all generated bindings
    • networks: they were being set for all contracts that we needed Contract::deployed() for (with the exception of WETH, but that was a very small amount of data to add).
    • bytecode: For contracts that are only needed as an interface and are not deployed for E2E
  • Creates Sushiswap* to UniswapV2 symbolic links
    • This is done because they share an interface
    • The Sushiswap contracts weren't getting automatically vendored by the vendor.rs script.

Furthermore, two contracts for which we had bindings but are unused have been removed (UniswapV2Pair and SushiswapV2Pair, since we use IUniswapLikePair for both those contracts).

Test Plan

Added new tests to ensure that contracts have deployment information configured for all networks that we care about (to avoid another incident like we had in staging this morning).

Otherwise, no service changes, so CI will hopefully catch all issues.

@nlordell nlordell requested a review from a team May 20, 2021 14:43
Copy link
Contributor

@vkgnosis vkgnosis left a comment

Choose a reason for hiding this comment

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

I was confused about contracts/artifacts/SushiswapV2Router02.json because the github diff renders this as if it c ontained the single line UniswapV2Router02.json. Actually it's a symbolic link to that file.

@nlordell
Copy link
Contributor Author

I was confused about contracts/artifacts/SushiswapV2Router02.json because the github diff renders this as if it c ontained the single line UniswapV2Router02.json. Actually it's a symbolic link to that file.

Added info to the PR description

Copy link
Contributor

@bh2smith bh2smith left a comment

Choose a reason for hiding this comment

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

Really cool. Will look over the fact stuff again tomorrow morning.

contracts/build.rs Show resolved Hide resolved
contracts/src/bin/deploy.rs Show resolved Hide resolved
contracts/src/bin/vendor.rs Show resolved Hide resolved
@bh2smith
Copy link
Contributor

One general comment, it looks like this kinda tooling could eventually fit more naturally in ethcontract. Given that it has become quite useful and sophisticated, it would be more easily accessible for other projects.

@nlordell
Copy link
Contributor Author

looks like this kinda tooling could eventually fit more naturally in ethcontract

cowprotocol/ethcontract-rs#514

contracts/src/bin/vendor.rs Show resolved Hide resolved
}

#[test]
fn deployment_addresses() {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice, I wonder how we can encourage people to add tests here whenever a new contract is added.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could somehow generate these lines from a constant list

include!(concat!(env!("OUT_DIR"), "/WETH9.rs"));

and include in this test and assertion each element from the list was verified here...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mergify mergify bot merged commit 982246f into main May 25, 2021
@mergify mergify bot deleted the balancer-vault-vendoring branch May 25, 2021 13:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants