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

Add Relayer V5 #401

Merged
merged 6 commits into from
Mar 31, 2023
Merged

Add Relayer V5 #401

merged 6 commits into from
Mar 31, 2023

Conversation

brunoguerios
Copy link
Member

@brunoguerios brunoguerios commented Mar 30, 2023

  • Add RelayerV5 to the SDK
  • Update generalisedJoin to use RelayerV5
  • Update generalisedExit to use RelayerV5

Note: relayer addresses were taken from here

@brunoguerios brunoguerios changed the title Generalised Exit - Proportional Exit Add Relayer V5 Mar 30, 2023
Copy link
Contributor

@gmbronco gmbronco left a comment

Choose a reason for hiding this comment

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

Looks good, balancerRelayerInterface encoding needs to be updated here as well: https://github.com/balancer/balancer-sdk/blob/develop/balancer-js/src/modules/swaps/joinAndExit.ts/#L1052-L1055

balancer-js/src/lib/constants/config.ts Show resolved Hide resolved
balancer-js/src/modules/contracts/contracts.module.ts Outdated Show resolved Hide resolved
Copy link
Member

@johngrantuk johngrantuk left a comment

Choose a reason for hiding this comment

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

Should also add RelayerV5 to src/modules/contracts/implementations/relayer.ts and make sure we're using most recent contract in tests, examples, etc.

@brunoguerios
Copy link
Member Author

Should also add RelayerV5 to src/modules/contracts/implementations/relayer.ts and make sure we're using most recent contract in tests, examples, etc.

This file was one of the reasons I asked if we should keep instantiating as Contract types or as RelayerV5 type. I chose to instantiate as RelayerV5 directly on contracts.module.ts.
Following that logic, I don't even see much benefit of having this implementations folder/files.
Why do we have them?

@johngrantuk
Copy link
Member

I can't remember the original reason TBH. Looks like some have some additional logic. I think if we're keeping them then we should update to include V5 to keep consistency. If we're removing them then it might be better to do it in another PR.

15773550
);

// Removed test that started failing after updating to a more recent blockNumber
Copy link
Member Author

Choose a reason for hiding this comment

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

I had to update to a more recent block due to relayerV5 and this test started failing. I tried figuring out why, but it was taking too long and I decided it would be better to check before spending more time on it.
@johngrantuk - this was the only test with a custom blockNumber - do you remember why that was the case?

Copy link
Member

Choose a reason for hiding this comment

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

I don't actually remember. I wanted to work on refactoring joinAndExits a bit next week so can investigate then. Should be ok to ignore for now.

@brunoguerios brunoguerios merged commit 9bca1b1 into develop Mar 31, 2023
@brunoguerios brunoguerios deleted the relayer-v5 branch March 31, 2023 12:52
@johngrantuk johngrantuk mentioned this pull request Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants