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

remove sdk.Context param in PoolI interface function #1533

Closed
catShaark opened this issue May 18, 2022 · 4 comments
Closed

remove sdk.Context param in PoolI interface function #1533

catShaark opened this issue May 18, 2022 · 4 comments

Comments

@catShaark
Copy link
Contributor

catShaark commented May 18, 2022

Background

  • I think PoolI interface functions should not take in a sdk.Context param because those functions don't need to access or change the state. Accessing or changing the state is done via gamm keeper's methods.
  • I've checked all the PoolI interface function implementations, none of them actually uses the sdk.Context passed in.

Suggested Change

We remove sdk.Context param from all the PoolI interface function

@alexanderbez
Copy link
Contributor

I believe I recall @ValarDragon stating that other pool implementations might need the context for state mutations.

@ValarDragon
Copy link
Member

I'd like to keep ctx there, it enables features like:

@catShaark
Copy link
Contributor Author

catShaark commented May 18, 2022

If so, should "get" functions like GetSwapFee or GetExitFee which has nothing to do with state mutations takes in sdk.Context. Also, I see that GetTotalShares don't need ctx, but other "get" functions need ctx, is there a reason behind this.

@ValarDragon
Copy link
Member

ValarDragon commented May 25, 2022

We probably need this for GetSwapFee for volatility aware AMM's / harm mitigation, I don't see a reason why GetTotalShares would need to depend on state, except if a pool decided to store it in state rather than it struct. (We could add ctx there for simplicity)

Repository owner moved this from Needs Review 🔍 to Done ✅ in Osmosis Chain Development May 27, 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

No branches or pull requests

3 participants