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

test(x/gamm): remove liquidity events #2186

Merged
merged 11 commits into from
Aug 3, 2022
Merged

Conversation

p0mvn
Copy link
Member

@p0mvn p0mvn commented Jul 21, 2022

Part of: #1942

What is the purpose of the change

Tests remove liquidity events that should be emitted when a pool is exited.

Similarly to #2141, I noticed that they were emitted twice:

  1. at the end of the message server
  2. right after a pool state is updated.

I removed 1. while keeping 2.

No breaking changes.

Passthrough changes

  • moved AssertEventEmitted test helper to be defined on the suite. It is also useful for testing events in other modules
  • cleaned up some tests by creating variables for keepers and ctx
  • added suite.Run() to some table-driven tests

Testing and Verifying

Documentation and Release Note

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

@github-actions github-actions bot added C:app-wiring Changes to the app folder C:x/gamm Changes, features and bugs related to the gamm module. C:x/superfluid labels Jul 21, 2022
@@ -483,43 +490,50 @@ func (suite *KeeperTestSuite) TestExitPool() {
}

for _, test := range tests {
suite.SetupTest()
suite.Run(test.name, func() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Note to reviewers: the diff is large because of indentation change due to suite.Run

@p0mvn p0mvn marked this pull request as ready for review July 21, 2022 23:52
@p0mvn p0mvn requested a review from a team July 21, 2022 23:52
@ValarDragon ValarDragon added the A:backport/v10.x backport patches to v10.x branch label Jul 22, 2022
@p0mvn p0mvn added A:backport/v11.x backport patches to v11.x branch and removed A:backport/v10.x backport patches to v10.x branch labels Jul 29, 2022
x/gamm/keeper/internal/events/emit_test.go Outdated Show resolved Hide resolved
x/gamm/keeper/msg_server_test.go Outdated Show resolved Hide resolved
shareInAmount: sdk.NewInt(shareOut),
tokenOutMins: sdk.NewCoins(),
expectedRemoveLiquidityEvents: 1,
expectedMessageEvents: 3, // 1 gamm + 2 tendermint.
Copy link
Member

Choose a reason for hiding this comment

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

just for my learning, why are there two TM messages emitted here?

Copy link
Member Author

Choose a reason for hiding this comment

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

These are not TM, the comment is wrong, and I'm going to change this. This happens because other keeper methods emit these events. For example, bank send:
https://github.com/cosmos/cosmos-sdk/blob/71035879e4af4551b9bcf16f79bc52785793602e/x/bank/keeper/send.go#L213-L215

I think the bank send is wrong and should instead have its message emitted in the message server so that we don't triple-count it here.

x/gamm/keeper/pool_service_test.go Outdated Show resolved Hide resolved
@github-actions github-actions bot added the C:docs Improvements or additions to documentation label Aug 3, 2022
@p0mvn
Copy link
Member Author

p0mvn commented Aug 3, 2022

Planning on merging this once CI passes. All comments are addressed and no API or state-breaking changes

CHANGELOG.md Outdated Show resolved Hide resolved
p0mvn and others added 2 commits August 3, 2022 09:08
Co-authored-by: Aleksandr Bezobchuk <[email protected]>
@p0mvn p0mvn merged commit 40f19a3 into main Aug 3, 2022
@p0mvn p0mvn deleted the roman/remove-liq-gamm-events branch August 3, 2022 18:09
mergify bot pushed a commit that referenced this pull request Aug 3, 2022
* test(x/gamm): remove liquidity events

* remove capacity pre-allocation

* Update x/gamm/keeper/internal/events/emit_test.go

Co-authored-by: Adam Tucker <[email protected]>

* Update x/gamm/keeper/msg_server_test.go

Co-authored-by: Adam Tucker <[email protected]>

* Update x/gamm/keeper/pool_service_test.go

Co-authored-by: Adam Tucker <[email protected]>

* shareIn

* update event comments

* changelog

* Update CHANGELOG.md

Co-authored-by: Aleksandr Bezobchuk <[email protected]>

* add docs

Co-authored-by: Adam Tucker <[email protected]>
Co-authored-by: Aleksandr Bezobchuk <[email protected]>
(cherry picked from commit 40f19a3)

# Conflicts:
#	CHANGELOG.md
#	x/gamm/keeper/pool_service_test.go
#	x/superfluid/keeper/unpool_test.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:backport/v11.x backport patches to v11.x branch C:app-wiring Changes to the app folder 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.

4 participants