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

Feature/add details to join exit pool reponse #1886

Conversation

Ehsan-saradar
Copy link
Contributor

What is the purpose of the change

This PR adds more information to response of join/exit pool handlers so the caller of the transaction in ICA receives these information on acknowledgment.

Brief Changelog

  • Added tokenIn sdk.Coins, sharesOut sdk.Int to return of JoinPoolNoSwap
  • share_out_amount and token_in fields to MsgJoinPoolResponse
  • token_out field to MsgExitPoolResponse

Testing and Verifying

This change added tests and can be verified as follows:

  • Extended integration test for TestJoinPoolNoSwap

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? (yes)
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? (no)
  • How is the feature or change documented? (not documented)

@Ehsan-saradar Ehsan-saradar requested a review from a team June 28, 2022 08:00
@github-actions github-actions bot added C:x/gamm Changes, features and bugs related to the gamm module. C:x/superfluid labels Jun 28, 2022
Copy link
Member

@mattverse mattverse left a comment

Choose a reason for hiding this comment

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

Thanks for this PR! I personally think adding responses to messages are super useful.

Can we make sure we add new test cases and checks for the new fields to ensure they return expected values?

Also, can you drop the changes for go.mod and go.sum as well? Otherwise, logic itself
lgtm!

@@ -296,7 +298,7 @@ func (suite *KeeperTestSuite) TestJoinPoolNoSwap() {
keeper := suite.App.GAMMKeeper
// Test the "tokenInMaxs"
// In this case, to get the 50 * OneShare amount of share token, the foo, bar token are expected to be provided as 5000 amounts.
err := keeper.JoinPoolNoSwap(suite.Ctx, suite.TestAccs[1], poolId, types.OneShare.MulRaw(50), sdk.Coins{
_, _, err := keeper.JoinPoolNoSwap(suite.Ctx, suite.TestAccs[1], poolId, types.OneShare.MulRaw(50), sdk.Coins{
Copy link
Member

Choose a reason for hiding this comment

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

Can we add check and test ShareOutAmount and TokenIn returned here is as expected?

@@ -493,7 +495,7 @@ func (suite *KeeperTestSuite) TestJoinPoolExitPool_InverseRelationship() {
balanceBeforeJoin := suite.App.BankKeeper.GetAllBalances(suite.Ctx, joinPoolAcc)
fmt.Println(balanceBeforeJoin.String())

err = suite.App.GAMMKeeper.JoinPoolNoSwap(suite.Ctx, joinPoolAcc, poolId, tc.joinPoolShareAmt, sdk.Coins{})
_, _, err = suite.App.GAMMKeeper.JoinPoolNoSwap(suite.Ctx, joinPoolAcc, poolId, tc.joinPoolShareAmt, sdk.Coins{})
Copy link
Member

Choose a reason for hiding this comment

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

ditto:Can we add check and test ShareOutAmount and TokenIn returned here is as expected?

@ValarDragon
Copy link
Member

ValarDragon commented Jun 28, 2022

Needs changelog entry as well, but feature looks good to me!

@ValarDragon
Copy link
Member

Ok so that this doesn't get blocked, going to work on a PR into this branch that should suffice!

@ValarDragon ValarDragon self-assigned this Jul 6, 2022
@ValarDragon
Copy link
Member

quasar-finance#2

Copy link
Member

@mattverse mattverse left a comment

Choose a reason for hiding this comment

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

Reviewed the changes made in quasar-finance#2 Let's merge this once quasar-finance#2 gets merged!

@github-actions github-actions bot added the C:docs Improvements or additions to documentation label Jul 7, 2022
@ValarDragon ValarDragon merged commit b23a4fd into osmosis-labs:main Jul 7, 2022
@czarcas7ic czarcas7ic mentioned this pull request Aug 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:docs Improvements or additions to documentation C:x/gamm Changes, features and bugs related to the gamm module. C:x/superfluid
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants