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.

Sign up for GitHub

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

Jump to bottom

x/gamm: Make all internal set functions private #1945

Closed
AlpinYukseloglu opened this issue Jul 1, 2022 · 2 comments · Fixed by #2013
Closed

x/gamm: Make all internal set functions private #1945

AlpinYukseloglu opened this issue Jul 1, 2022 · 2 comments · Fixed by #2013
Assignees
@AlpinYukseloglu
Labels
C:x/gamm Changes, features and bugs related to the gamm module. T:code-hygiene

Comments

@AlpinYukseloglu
Copy link
Contributor

Background

I believe these functions were only public because they needed to be for an older SDK version. I think it would be appropriate to make them private now so that they aren't exposed to other modules.

cc: @ValarDragon @alexanderbez @p0mvn for discussion

Suggested Design

  • Convert all the following public GAMM set functions to private:
    SetParams
    SetPool
    SetNextPoolNumber
    SetTotalLiquidity
    SetDenomLiquidity

Acceptance Criteria

  • All existing tests pass
@AlpinYukseloglu AlpinYukseloglu added T:code-hygiene C:x/gamm Changes, features and bugs related to the gamm module. labels Jul 1, 2022
@ValarDragon ValarDragon moved this to Needs Review 🔍 in Osmosis Chain Development Jul 1, 2022
@AlpinYukseloglu AlpinYukseloglu self-assigned this Jul 3, 2022
@AlpinYukseloglu
Copy link
Contributor Author

@ValarDragon it seems as though a bunch of our gamm tests use the gamm keeper set up in suite.App, which is only allowed to use public keeper methods. For example, if we make SetParams private, the following call has no way to access it:

osmosis/x/gamm/keeper/pool_service_test.go

Lines 212 to 213 in 8f018a6

keeper := suite.App.GAMMKeeper
keeper.SetParams(suite.Ctx, types.Params{

Any ideas on how we can get around this?

@mattverse
Copy link
Member

use export_test.go if there's need to test private functions!

@AlpinYukseloglu AlpinYukseloglu mentioned this issue Jul 10, 2022
Merged
@mergify mergify bot closed this as completed in #2013 Jul 18, 2022
mergify bot pushed a commit that referenced this issue Jul 18, 2022
@AlpinYukseloglu
x/gamm: Make all internal set functions private (#2013)
7fb5f82 
Closes: #1945 

## What is the purpose of the change

I believe these functions were only public because they needed to be for an older SDK version. I think it would be appropriate to make them private now so that they aren't exposed to other modules.

## Brief Changelog

- Made the following functions private:
`SetParams`
`SetPool`
`SetTotalLiquidity`
`SetDenomLiquidity`
- Commented out old upgrade code that prevented `SetParams` and `SetPool` from being made private
- Removed use of `SetParams` in a superfluid test

## 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? (no)
  - Is a relevant changelog entry added to the `Unreleased` section in `CHANGELOG.md`? (no)
  - How is the feature or change documented? (not documented)
Repository owner moved this from Needs Review 🔍 to Done ✅ in Osmosis Chain Development Jul 18, 2022
mergify bot pushed a commit that referenced this issue Jul 18, 2022
@AlpinYukseloglu @mergify
x/gamm: Make all internal set functions private (#2013)
f5581fd 
Closes: #1945

## What is the purpose of the change

I believe these functions were only public because they needed to be for an older SDK version. I think it would be appropriate to make them private now so that they aren't exposed to other modules.

## Brief Changelog

- Made the following functions private:
`SetParams`
`SetPool`
`SetTotalLiquidity`
`SetDenomLiquidity`
- Commented out old upgrade code that prevented `SetParams` and `SetPool` from being made private
- Removed use of `SetParams` in a superfluid test

## 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? (no)
  - Is a relevant changelog entry added to the `Unreleased` section in `CHANGELOG.md`? (no)
  - How is the feature or change documented? (not documented)

(cherry picked from commit 7fb5f82)

# Conflicts:
#	CHANGELOG.md
#	app/upgrades/v4/upgrade_test.go
#	app/upgrades/v4/upgrades.go
#	x/gamm/genesis.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@AlpinYukseloglu AlpinYukseloglu

Labels
C:x/gamm Changes, features and bugs related to the gamm module. T:code-hygiene
Projects
Osmosis Chain Development
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

x/gamm: Make all internal set functions private osmosis-labs/osmosis
2 participants
@mattverse @AlpinYukseloglu