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

Refactor gamm hooks to handle error instead of panics #2561

Closed
wants to merge 3 commits into from

Conversation

Anmol1696
Copy link

@Anmol1696 Anmol1696 commented Sep 1, 2022

Part of: #2520

What is the purpose of the change

  • GAMM hooks deal with errors by panicing on errors, handle errors in the hook functions.
  • Hooks errors can now be handled with ApplyFuncIfNoError
  • This implementation allows hooks interface functions to return an error.
  • If functions return an error any state changes will not be written to the state.

Brief Changelog

  • Update the interface for GAMM hooks, to add return type error
type GammHooks interface {
  AfterPoolCreated(ctx sdk.Context, sender sdk.AccAddress, poolId uint64) error
  AfterJoinPool(ctx sdk.Context, sender sdk.AccAddress, poolId uint64, enterCoins sdk.Coins, shareOutAmount sdk.Int) error
  AfterExitPool(ctx sdk.Context, sender sdk.AccAddress, poolId uint64, shareInAmount sdk.Int, exitCoins sdk.Coins) error
  AfterSwap(ctx sdk.Context, sender sdk.AccAddress, poolId uint64, input sdk.Coins, output sdk.Coins) error
}
  • Update methods that implement the GammHooks interface.

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)

@github-actions github-actions bot added C:x/gamm Changes, features and bugs related to the gamm module. C:x/pool-incentives C:x/twap Changes to the twap module labels Sep 1, 2022
@github-actions github-actions bot added the C:docs Improvements or additions to documentation label Sep 1, 2022
@Anmol1696 Anmol1696 marked this pull request as ready for review September 1, 2022 10:35
@Anmol1696 Anmol1696 requested review from a team and puneet2019 September 1, 2022 10:35
}
err := osmoutils.ApplyFuncIfNoError(ctx, wrappedHookFn)
if err != nil {
ctx.Logger().Error(fmt.Sprintf("error in gamm hook %v", err))
Copy link
Member

Choose a reason for hiding this comment

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

So we can do this approach if panic(err) here for now

@ValarDragon
Copy link
Member

ValarDragon commented Sep 14, 2022

If its alright, I'd like to close all but the mint hook switch for now.

This logic is pretty sensitive, and I'd prefer to not load too much state in refining this pretty brittle interface right now. Bez and I are working with Marko / SDK team to get more satisfactory framework-level answers for this problem, and would rather just reason about that + a switch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:docs Improvements or additions to documentation C:x/gamm Changes, features and bugs related to the gamm module. C:x/pool-incentives C:x/twap Changes to the twap module
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants