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

[x/gamm][Refactor] Follow-on cleanup to Gamm Refactor #1129

Merged
merged 6 commits into from
Mar 23, 2022

Conversation

ValarDragon
Copy link
Member

@ValarDragon ValarDragon commented Mar 23, 2022

Followup cleanup from the gamm refactor, now deleting unused code, and shuffling around where structs are defined, and if functions are publicly exposed. This PR has no logic updates, it just moves definitions around.

It should be reviewable commit by commit.

  • deletes math.go and math_test.go. math_test's intended logic should imo be re-written from scratch for balancer types, it did not really seem like it had a useful form to me.
  • delete the legacy pool interface
  • Move event creation to types/events.go
  • Move poolasset.pb.go to pool-models/balancer
  • Move balancer weight validation constants & function

For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

@codecov-commenter
Copy link

codecov-commenter commented Mar 23, 2022

Codecov Report

Merging #1129 (e4bc4ed) into main (f1389ee) will increase coverage by 0.28%.
The diff coverage is 39.68%.

@@            Coverage Diff             @@
##             main    #1129      +/-   ##
==========================================
+ Coverage   19.90%   20.19%   +0.28%     
==========================================
  Files         200      199       -1     
  Lines       25498    25062     -436     
==========================================
- Hits         5076     5061      -15     
+ Misses      19515    19074     -441     
- Partials      907      927      +20     
Impacted Files Coverage Δ
osmomath/math.go 64.44% <0.00%> (-1.47%) ⬇️
x/gamm/client/cli/query.go 42.26% <ø> (+3.71%) ⬆️
x/gamm/keeper/grpc_query.go 39.58% <ø> (+0.80%) ⬆️
x/gamm/keeper/invariants.go 0.00% <0.00%> (ø)
x/gamm/keeper/keeper.go 60.86% <ø> (-20.39%) ⬇️
x/gamm/keeper/pool.go 68.00% <ø> (ø)
x/gamm/pool-models/balancer/marshal.go 78.57% <ø> (ø)
x/gamm/pool-models/balancer/tx.pb.go 0.77% <0.00%> (ø)
x/gamm/types/events.go 0.00% <0.00%> (ø)
x/gamm/types/pool.go 0.00% <ø> (ø)
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 196a644...e4bc4ed. Read the comment docs.

@ValarDragon ValarDragon mentioned this pull request Mar 23, 2022
6 tasks
@ValarDragon ValarDragon force-pushed the dev/remove_obsolete_code branch from 8bcf054 to 1802861 Compare March 23, 2022 03:15
@ValarDragon ValarDragon force-pushed the dev/remove_obsolete_code branch from 1802861 to e4bc4ed Compare March 23, 2022 03:16
@ValarDragon ValarDragon marked this pull request as ready for review March 23, 2022 03:27
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

Don't have much context here, but it seems like we just moved things around for clarity and better architecture, so LGTM 👍

osmomath/math.go Show resolved Hide resolved
proto/osmosis/gamm/pool-models/balancer/balancerPool.proto Outdated Show resolved Hide resolved
x/gamm/pool-models/balancer/balancer_pool.go Show resolved Hide resolved
x/gamm/pool-models/balancer/constants.go Outdated Show resolved Hide resolved
@ValarDragon
Copy link
Member Author

Thanks for pushing for more docs, added some for everything you pointed out!

@alexanderbez
Copy link
Contributor

image

@ValarDragon ValarDragon merged commit d143d83 into main Mar 23, 2022
@ValarDragon ValarDragon deleted the dev/remove_obsolete_code branch March 23, 2022 15:45
@p0mvn p0mvn mentioned this pull request May 9, 2022
12 tasks
@p0mvn p0mvn changed the title Follow-on cleanup to Gamm Refactor [x/gamm][Refactor] Follow-on cleanup to Gamm Refactor May 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants