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.

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

Vendor Stable Pool V2 Artifacts #371

Merged
merged 14 commits into from
Jul 19, 2022
Merged

Vendor Stable Pool V2 Artifacts #371

merged 14 commits into from
Jul 19, 2022

Conversation

bh2smith
Copy link
Contributor

@bh2smith bh2smith commented Jul 18, 2022

Based on #368, intended to close #343 as the second step in a two step process.

This PR vendors the new versions of Stable Pool Factory and StablePool (names here are the originals with V2 on the end) as abi_only.

Note that, we use the bytecode to deploy the contract for the e2e tests: We may want to ask balancer to supply it. However, in the meantime we don't intend on adding any e2e tests for this particular factory contract.

New one.
https://github.com/balancer-labs/balancer-v2-monorepo/blob/903d34e491a5e9c5d59dabf512c7addf1ccf9bbd/pkg/deployments/tasks/20220609-stable-pool-v2/abi/StablePoolFactory.json

Old one.
https://github.com/balancer-labs/balancer-v2-monorepo/blob/ad1442113b26ec22081c2047e2ec95355a7f12ba/pkg/deployments/tasks/20210624-stable-pool/abi/StablePoolFactory.json

TEST PLAN

Checkout main and run the orderbook: expect the warning as described in #343.

Checkout this branch and run the orderbook: Notice that there is no more warning.

Furthermore, I have validated that these pools are being included as follows:

Run the unit test: sources::balancer_v2::pool_fetching::tests::balancer_pool_fetcher_print

with the pair set as

        let pair = TokenPair::new(
            addr!("5c6ee304399dbdb9c8ef030ab642b10820db8f56"),
            addr!("f24d8651578a55b0c119b9910759a351a3458895"),
        )

and an additional print statement:

dbg!("{:?}", fetched_pools_by_id);

which showed that this pool 0x2d011adf89f0576c9b722c28269fcb5d50c2d179 was indeed categorized as a stable pool.

@bh2smith bh2smith changed the title Stable Pool Factory V2 Vendor Stable Pool V2 Artifacts Jul 18, 2022
@MartinquaXD
Copy link
Contributor

Address the build error...

If you just want to "pretend" to use a result to satisfy the type checker you can do this:

let _ = compute_result_i_dont_care_about();

If you are interested in the result you have to give it a name:

let some_name = compute_result_i_care_about();

@bh2smith
Copy link
Contributor Author

bh2smith commented Jul 18, 2022

Thank @MartinquaXD for taking a look at this! Now that you're interested I can provide a bit more context on this (auto generated file):

  1. The code generated here is done automatically by our build, so I really don't think we should be editing this.
  2. There is analogous code in the other generated file, however this particular line is appearing twice in the new one and once without anything in the let statement.

Screenshot 2022-07-18 at 12 16 46 PM

More specifically: we see this in the new factory contract:
Screenshot 2022-07-18 at 12 19 38 PM

Which doesn't appear to be part of the other one. In both we have this:

Screenshot 2022-07-18 at 12 20 01 PM

From what I can tell this event FactoryDisabled is new and perhaps it does not have any return values... So I think this might be a corner case for ethcontract about how to build event data for an event with no emmitted values.

@MartinquaXD
Copy link
Contributor

I didn't see that this came from generated code. 🤦‍♂️
In that case I agree that you shouldn't modify that code manually.

@bh2smith
Copy link
Contributor Author

bh2smith commented Jul 18, 2022

My hunch appears to be correct: This event doesn't come with any values,

Screenshot 2022-07-18 at 12 24 53 PM

We may need to update ethcontract to support this (cc @nlordell).

I have created an issue in ethcontract-rs: cowprotocol/ethcontract-rs#796

@vkgnosis
Copy link
Contributor

It looks like this is a clippy warning, so no hard build error. We don't have to block this PR on fixing this in ethcontract (but it should be fixed eventually).
We can work around this by putting

// Workaround for ethcontract generated code running into this warning.
#![allow(clippy::let_unit_value)]

at the very top of crates/contracts/src/lib.rs.

@bh2smith bh2smith marked this pull request as ready for review July 19, 2022 12:48
@bh2smith bh2smith requested a review from a team as a code owner July 19, 2022 12:48
Base automatically changed from stable-math-update to main July 19, 2022 12:57
@@ -108,6 +111,48 @@ impl FactoryIndexing for BalancerV2StablePoolFactory {
}
}

// TODO - this probably doesn't need to be so clearly duplicated.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ben and I talked about this. I felt this was fine for now. When there is a V3 it might be worth it to deduplicate.

@bh2smith bh2smith merged commit d596a03 into main Jul 19, 2022
@bh2smith bh2smith deleted the stable-pool-factory2 branch July 19, 2022 13:09
@github-actions github-actions bot locked and limited conversation to collaborators Jul 19, 2022
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.

Investigate "found pools that don't correspond to any known Balancer pool factory "
3 participants