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

Gamm Spec Out V3 - Modulize File Structure #541

Merged
merged 31 commits into from
Dec 21, 2021

Conversation

mattverse
Copy link
Member

@mattverse mattverse commented Oct 18, 2021

Description

This PR is the 3rd version of Gamm spec out, including the main generalization and spec out of gamm module by seperating pool-models/balancer package with types package. This PR includes the following changes in detail:

  1. Proto Changes
  • divides proto packages into two different directories: proto/osmosis/gamm/v1beta, proto/osmosis/gamm/pool-models/balancer where gamm/v1beta directory includes protobufs that could be used for generalized usage(such as PoolAsset), and gamm/pool-models/balancer directory only includes protobufs limited to the scope of a balancer pool.
  • proto/osmosis/gamm/pool-models/balancer also includes tx.proto, which includes Msgs that uses protos defined in the package.
  1. gamm module structure changes
  • gamm module has a new directory, pool-models at top level. pool-models would include packages for each specific pool models.
  • each specific pool packages defined in pool-models would be importing gamm's types package, but should not vice-versa(due to import cycles in go lang)
  1. pool-models requirements
    A package defined under pool-models directory(e.g balancer package in this PR) should meet the following requirements.
  • implement poolI for the specific pool
  • have marshal.go file where marshaling would happen in accordance to the pool structure it has.
  1. requirements for defining msgs in a package under pool-models.
    In the cases of where tx or msgs that has dependency on the created pool model package, the following requirements has to be specified.
  • create tx.proto under the correct proto directory
  • create msgs.go under the specified package, implement the necessary methods(e.g. Route(), Type(), ValidateBasic()).
  • create codec.go under the specified package for the model, register legacy animo codec and interface each using RegisterLegacyAminoCodec, RegisterInterfaces respectively.
  • register newly created codec in module.go in gamm module.
  1. PoolParams Cli
    Even though all pools would have their own poolParams proto, query for PoolParams still exists in generalized form(in the form of Any). Thereby, grpc_query.go should be implementing poolParams type of the specific module using switch cases.

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

go.mod Outdated Show resolved Hide resolved
@mattverse mattverse changed the title Mattverse/gamm spec out v3.1 Gamm Spec Out V3 - Modulize File Structure Oct 19, 2021
Base automatically changed from mattverse/gamm-spec-out to main October 19, 2021 15:41
@ValarDragon
Copy link
Member

Why is this PR against main instead of the v2 branch

@mattverse
Copy link
Member Author

my bad - should be v2 branch

@mattverse mattverse changed the base branch from main to mattverse/gamm-spec-out-v2 October 19, 2021 20:13
return nil
}

func ValidateFutureGovernor(governor string) error {
Copy link
Member

Choose a reason for hiding this comment

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

This should be defined in the pool interface folder I think

Copy link
Member Author

Choose a reason for hiding this comment

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

What's the pool interface folder? Did you mean types.go where poolI used to be defined?
types.go was deleted, so moved the method to balancer_pool.go for now!

@daniel-farina daniel-farina added this to the 2021 - December Milestone milestone Dec 2, 2021
Base automatically changed from mattverse/gamm-spec-out-v2 to main December 16, 2021 14:13
@@ -1,5 +1,5 @@
syntax = "proto3";
package osmosis.gamm.v1beta1;
package osmosis.gamm.poolmodels;
Copy link
Member

Choose a reason for hiding this comment

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

should this still be behind a v1beta1?

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you referring to the entire file itself? If my understanding is right upon your question, pool-models is declared as a whole new package in gamm, thats where this is coming from.

Copy link
Member

Choose a reason for hiding this comment

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

I see, tbh I don't really understand the v1beta1 stuff. Lets make an issue for this question, and then ask regen folks post-holidays

Copy link
Member Author

Choose a reason for hiding this comment

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

haha. I'm supposing its just a legacy in structuring the proto files from the cosmos-sdk and that it does not have any additional functionallities. I didn't have any issues when I wasn't using v1beta1 as a package file structure, they should be good to be removed as well

proto/osmosis/gamm/pool-models/balancer/balancerPool.proto Outdated Show resolved Hide resolved
proto/osmosis/gamm/v1beta1/query.proto Show resolved Hide resolved
x/gamm/genesis.go Outdated Show resolved Hide resolved
@mattverse
Copy link
Member Author

@ValarDragon Can you also review the poolI interface fields? Generalizing the gamm structure, I don't think we need all the methods currently in poolI and want to remove some of the unnecessary methods from poolI. Whats your opinion on this?

@codecov-commenter
Copy link

codecov-commenter commented Dec 20, 2021

Codecov Report

Merging #541 (eccdfee) into main (b03cc8c) will decrease coverage by 0.24%.
The diff coverage is 23.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #541      +/-   ##
==========================================
- Coverage   18.39%   18.14%   -0.25%     
==========================================
  Files         173      178       +5     
  Lines       24321    24453     +132     
==========================================
- Hits         4473     4437      -36     
- Misses      19086    19257     +171     
+ Partials      762      759       -3     
Impacted Files Coverage Δ
x/gamm/keeper/pool_service.go 62.35% <ø> (ø)
x/gamm/types/codec.go 40.74% <ø> (-1.20%) ⬇️
x/gamm/types/msgs.go 71.42% <ø> (-10.66%) ⬇️
x/gamm/types/pool.go 0.00% <0.00%> (-69.32%) ⬇️
x/gamm/types/pool_asset.go 0.00% <0.00%> (-68.52%) ⬇️
x/gamm/types/query.pb.go 0.84% <0.00%> (+<0.01%) ⬆️
x/gamm/types/tx.pb.go 0.71% <0.00%> (+<0.01%) ⬆️
x/gamm/pool-models/balancer/tx.pb.go 0.81% <0.81%> (ø)
x/gamm/types/pool.pb.go 1.00% <1.00%> (ø)
x/gamm/keeper/grpc_query.go 41.29% <18.75%> (+0.04%) ⬆️
... and 13 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 b03cc8c...eccdfee. Read the comment docs.

Copy link
Collaborator

@mconcat mconcat left a comment

Choose a reason for hiding this comment

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

Looks good, just one question!

x/gamm/keeper/pool.go Show resolved Hide resolved
Copy link
Member

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

LGTM.

I think we just need to make followup issues for:

  1. Separating out general pool parameters into their own struct. (Swap fee, Exit fee)
  2. Removing PoolAsset as a struct, and moving all weight handling to be internal to balancer
  3. the .proto v1beta1 question.

Then I think we're good to merge this!

@@ -1,5 +1,5 @@
syntax = "proto3";
package osmosis.gamm.v1beta1;
package osmosis.gamm.poolmodels;
Copy link
Member

Choose a reason for hiding this comment

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

I see, tbh I don't really understand the v1beta1 stuff. Lets make an issue for this question, and then ask regen folks post-holidays

proto/osmosis/gamm/v1beta1/pool.proto Show resolved Hide resolved
proto/osmosis/gamm/v1beta1/query.proto Show resolved Hide resolved
x/gamm/client/cli/cli_test.go Outdated Show resolved Hide resolved
x/gamm/keeper/pool.go Show resolved Hide resolved
x/gamm/keeper/pool_service_test.go Outdated Show resolved Hide resolved

// assumes 0 < d < 1
func poolAssetsMulDec(base []PoolAsset, d sdk.Dec) []PoolAsset {
Copy link
Member

Choose a reason for hiding this comment

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

Do we just no longer need these pool asset arithmetic methods?

Copy link
Member Author

Choose a reason for hiding this comment

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

this was moved to balancer_pool.go bc the concept of pool weight moved to balancer package

@mattverse
Copy link
Member Author

@mattverse
Copy link
Member Author

mattverse commented Dec 21, 2021

@ValarDragon Could you also review poolI in pool.go?
Now that we're generalizing gamm module, I don't think we need all the method that was in poolI to still be there.
If we want to get this PR merged and mention this in a separate issue, I think that would also be fine!

@ValarDragon
Copy link
Member

Agreed that a lot of these methods should be changed.

I think it'd be easiest to do that when separating out the parameters. (So we can remove all the weight references, etc.)

I think we also may want secondary interfaces for different types of pools. (CFMM, batched-CFMM, etc.) but I think it's perhaps too early for doing that rn / unclear if we want to push that to cosmwasm.

@ValarDragon ValarDragon merged commit d8cc5b3 into main Dec 21, 2021
@ValarDragon ValarDragon deleted the mattverse/gamm-spec-out-v3.1 branch December 21, 2021 19:16
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.

6 participants