-
Notifications
You must be signed in to change notification settings - Fork 607
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
[e2e][CL]: pool migration #4690
Conversation
This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you! |
@ThanhNhann just wanted to check in to see what the plan for this was |
Hi @czarcas7ic, thanks for your ping, I'm updating it |
…han/e2e_pool_migration
…han/e2e_pool_migration
…han/e2e_pool_migration
yeah, I will update the code with @mattverse review today @czarcas7ic, just missing the notifications of this pr, so sorry for this |
@ThanhNhann please ping when r4r, thanks! |
It's good to review now @czarcas7ic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I have left some last comments that would be best if addressed, after that I think we should be good for merge
@ThanhNhann would like the point about 6 cases vs 4 addressed, then I think we are good to merge. |
thanks your review @czarcas7ic, I will update soon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work on this! I want to do one more review before merging though
balanceAfterJoin := s.addrBalance(node, poolJoinAddress) | ||
|
||
// The balancer join pool amount is the difference between the account balance before and after joining the pool. | ||
joinPoolAmt, _ = balanceBeforeJoin.SafeSub(balanceAfterJoin) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we use safesub here? there should never be a time when this value is negative
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
balanceAfterJoin will have another coin after joining pool so I think we should use SafeSub here
Hey, sorry for tall the back and forths on this. It looks like this test is significantly longer when compared to all the other E2E tests:
We might need to add some randomized logic to select a single migration path rather than checking every one every time. That or add some go routines in the test itself. 100s I think should be the max for one of these tests, and this is almost 5x that (not at all your fault, its a hefty test, just need to make this shorter) |
This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you! |
Closes: #4496
What is the purpose of the change
Add e2e test pool migrations from balancer/stableswap to CL
Brief Changelog
Testing and Verifying
(Please pick one of the following options)
This change is a trivial rework / code cleanup without any test coverage.
(or)
This change is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)
Documentation and Release Note
Unreleased
section inCHANGELOG.md
? (yes / no)x/<module>/spec/
) / Osmosis docs repo / not documented)