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): add liquidity event tests #2141

Merged
merged 5 commits into from
Jul 21, 2022
Merged

Conversation

p0mvn
Copy link
Member

@p0mvn p0mvn commented Jul 19, 2022

Part of: #1942

What is the purpose of the change

Tests add liquidity events that should be emitted when a pool is joined.

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.

Testing and Verifying

This change added tests

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:docs Improvements or additions to documentation C:x/gamm Changes, features and bugs related to the gamm module. labels Jul 19, 2022
x/gamm/spec/README.md Outdated Show resolved Hide resolved
x/gamm/spec/README.md Outdated Show resolved Hide resolved
x/gamm/spec/README.md Outdated Show resolved Hide resolved
x/gamm/spec/README.md Outdated Show resolved Hide resolved
@p0mvn p0mvn changed the title test(gamm): add liquidity events test(x/gamm): add liquidity events Jul 19, 2022
@p0mvn p0mvn changed the title test(x/gamm): add liquidity events test(x/gamm): add liquidity event tests Jul 19, 2022
@p0mvn p0mvn marked this pull request as ready for review July 19, 2022 15:34
@p0mvn p0mvn requested a review from a team July 19, 2022 15:34
@p0mvn p0mvn added the A:backport/v10.x backport patches to v10.x branch label Jul 20, 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.

LGTM!

Nice catch on removing the existing emit event!
I do have concern over not being able to intuitively track what events are happening from the msg server layer, rather have to follow inside methods to track what events are happening, but apart from that small concern I have LGTM!

x/gamm/keeper/msg_server.go Show resolved Hide resolved
x/gamm/keeper/internal/events/emit_test.go Show resolved Hide resolved
x/gamm/keeper/msg_server_test.go Outdated Show resolved Hide resolved
p0mvn and others added 3 commits July 21, 2022 13:24
Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com>
@p0mvn
Copy link
Member Author

p0mvn commented Jul 21, 2022

I do have concern over not being able to intuitively track what events are happening from the msg server layer, rather have to follow inside methods to track what events are happening, but apart from that small concern I have LGTM!

I think that's a good point. Let me refactor this at the very end, once I complete all the remaining gamm events. I will add this is an acceptance criteria to the #1942

@p0mvn
Copy link
Member Author

p0mvn commented Jul 21, 2022

I'm merging this since need to unblock further work and the change is not risky

@p0mvn p0mvn merged commit 4970f16 into main Jul 21, 2022
@p0mvn p0mvn deleted the roman/gamm-add-liq-events branch July 21, 2022 20:40
mergify bot pushed a commit that referenced this pull request Jul 21, 2022
* test(gamm): add liquidity events

* Apply suggestions from code review

* Update x/gamm/keeper/msg_server_test.go

Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com>

Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com>
(cherry picked from commit 4970f16)

# Conflicts:
#	x/gamm/keeper/pool_service_test.go
@p0mvn p0mvn added the A:backport/v11.x backport patches to v11.x branch label Jul 29, 2022
p0mvn added a commit that referenced this pull request Aug 2, 2022
* test(gamm): add liquidity events

* Apply suggestions from code review

* Update x/gamm/keeper/msg_server_test.go

Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com>

Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com>
p0mvn added a commit that referenced this pull request Aug 2, 2022
* test(gamm): add liquidity events

* Apply suggestions from code review

* Update x/gamm/keeper/msg_server_test.go

Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com>

Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com>
p0mvn added a commit that referenced this pull request Aug 2, 2022
* test(gamm): add liquidity events

* Apply suggestions from code review

* Update x/gamm/keeper/msg_server_test.go

Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com>

Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com>

Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:backport/v10.x backport patches to v10.x branch A:backport/v11.x backport patches to v11.x branch C:docs Improvements or additions to documentation C:x/gamm Changes, features and bugs related to the gamm module.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants