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 keeper delete obsolete files #2160

Merged
merged 12 commits into from
Aug 2, 2022
Merged

gamm keeper delete obsolete files #2160

merged 12 commits into from
Aug 2, 2022

Conversation

AlpinYukseloglu
Copy link
Contributor

@AlpinYukseloglu AlpinYukseloglu commented Jul 19, 2022

Closes: #2155

What is the purpose of the change

Cleanup of x/gamm/keeper

Brief Changelog

  • Lets view keeper.go as "generic SDK boilerplate", and move params.go code into keeper.go.
  • Remove usages of poolNumber everywhere, have it only be poolId.
  • Move SetStableSwapScalingFactors to stableswap
  • Lets delete marshal_bench_test.go - this existed for some misc. historical reason
  • Delete grpc_query_internal_test.go

Testing and Verifying

This change is a trivial rework / code cleanup without any test coverage.

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? (yes / no)
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? (yes / no)
  • How is the feature or change documented? (not applicable / specification (x/<module>/spec/) / Osmosis docs repo / not documented)

@AlpinYukseloglu AlpinYukseloglu added the C:x/gamm Changes, features and bugs related to the gamm module. label Jul 19, 2022
@github-actions github-actions bot added the C:simulator Edits simulator or simulations label Jul 20, 2022
@AlpinYukseloglu AlpinYukseloglu marked this pull request as ready for review July 20, 2022 01:41
@AlpinYukseloglu AlpinYukseloglu requested review from a team, p0mvn and ValarDragon July 20, 2022 01:41
@github-actions github-actions bot added the C:docs Improvements or additions to documentation 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.

Changelog entry should be changed to breaking changes, other than that LGTM!

CHANGELOG.md Outdated Show resolved Hide resolved
@ValarDragon ValarDragon added the A:backport/v10.x backport patches to v10.x branch label Jul 20, 2022
@@ -21,6 +21,6 @@ option go_package = "github.com/osmosis-labs/osmosis/v10/x/gamm/types";
message GenesisState {
repeated google.protobuf.Any pools = 1
[ (cosmos_proto.accepts_interface) = "PoolI" ];
uint64 next_pool_number = 2;
uint64 next_pool_id = 2;
Copy link
Contributor

@alexanderbez alexanderbez Jul 21, 2022

Choose a reason for hiding this comment

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

AFAIU, when you break proto like this, you technically have to bump the package version. i.e. osmosis.gamm.v2beta1;

Copy link
Member

Choose a reason for hiding this comment

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

So field name changes like this break clients and json serialization, but I think its fine to do so for genesis

cc @daniel-farina @czarcas7ic if field renames / genesis file breaks like this end up being problematic for integrators

@ValarDragon ValarDragon added the A:backport/v11.x backport patches to v11.x branch label Jul 31, 2022
@ValarDragon
Copy link
Member

Can we separate out the deletes here from the genesis field rename

Can easily get all the file deletes merged

@ValarDragon
Copy link
Member

Actually I'll just commit undoing that proto field rename, and we can re-open a new issue for talking about genesis breaks

@ValarDragon ValarDragon removed the A:backport/v10.x backport patches to v10.x branch label Aug 2, 2022
@ValarDragon ValarDragon merged commit 8cac090 into main Aug 2, 2022
@ValarDragon ValarDragon deleted the gamm-cleanup branch August 2, 2022 17:31
mergify bot pushed a commit that referenced this pull request Aug 2, 2022
* remove vestigial test files

* move params into keeper.go

* move stableswap scaling factor

* change pool number to pool id where applicable

* further pool number changes

* make proto level pool number changes

* add changelog entry

* remove completed TODO comment

* undo proto change

Co-authored-by: Dev Ojha <dojha@berkeley.edu>
(cherry picked from commit 8cac090)

# Conflicts:
#	CHANGELOG.md
#	go.mod
#	x/gamm/keeper/genesis.go
#	x/gamm/keeper/grpc_query.go
#	x/gamm/keeper/params.go
#	x/gamm/keeper/pool.go
#	x/gamm/keeper/pool_service.go
#	x/gamm/pool-models/stableswap/pool.go
#	x/gamm/simulation/sim_msgs.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:docs Improvements or additions to documentation C:simulator Edits simulator or simulations 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.

gamm keeper delete obsolete files
4 participants