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

Tokenfactory: Add Before send hooks #4382

Merged
merged 4 commits into from
Feb 23, 2023

Conversation

mattverse
Copy link
Member

@mattverse mattverse commented Feb 21, 2023

What is the purpose of the change

Adds BeforeSend hooks to token factory.

Background: We want to enable calls to cosmwasm contracts before send of a token factory denom.

We have registered BeforeSend hooks to the cosmos sdk a while ago, this PR implements the BeforeSend hook to the token factory module.

Whenever the before send hook in token factory is called, we check in state if a contract address has been registered for the specific denom being sent. If so, the contract is called before send via hooks.

Brief Changelog

  • Add BeforeSend Hooks to Token factory

Testing and Verifying

This change adds tests

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)

@mattverse mattverse added the V:state/compatible/backport State machine compatible PR, should be backported label Feb 21, 2023
@@ -63,6 +66,8 @@ message MsgBurn {
(gogoproto.moretags) = "yaml:\"amount\"",
(gogoproto.nullable) = false
];
string burnFromAddress = 3
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not related to the send hooks, right? Do we need these mint/burn changes?

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch! Srry meant to address this in a different PR must have been pushed together. Fixed


em := sdk.NewEventManager()

_, err = h.wasmkeeper.Sudo(ctx.WithEventManager(em), cwAddr, msgBz)
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably want to think about gas increase expectations for bank sends with tokenfactory denoms as the contract can consume unbounded gas

Copy link
Member Author

@mattverse mattverse Feb 21, 2023

Choose a reason for hiding this comment

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

Do you think its a possible option to gate usage of gas here?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, we've been discussing something similar for wasm-hooks: (#4195). For the ibc case, I think non-atomic multimsg is enough, but for send hooks I think we may want to have a hard-cap on how much gas those hooks are allowed to take (something like: https://github.com/cosmos/ibc-go/blob/main/modules/core/keeper/msg_server.go#L451) and have users submit with that limit.

There is no risk to the chain here though, it's more of a UX problem. If a contract (by malice or incompetence) has an infinite loop or just very high gas usage, users may need to submit their tx with increasing gas limits

Copy link
Member Author

Choose a reason for hiding this comment

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

I see I see, sounds fine for now for the UX tradeoff of gas problem, going ahead and merging this for now

func (suite *KeeperTestSuite) SetupTest() {
suite.Setup()
// Fund every TestAcc with two denoms, one of which is the denom creation fee
fundAccsAmount := sdk.NewCoins(sdk.NewCoin(types.DefaultParams().DenomCreationFee[0].Denom, types.DefaultParams().DenomCreationFee[0].Amount.MulRaw(100)), sdk.NewCoin(apptesting.SecondaryDenom, apptesting.SecondaryAmount))
for _, acc := range suite.TestAccs {
suite.FundAcc(acc, fundAccsAmount)
}

suite.contractKeeper = wasmkeeper.NewPermissionedKeeper(suite.App.WasmKeeper, SudoAuthorizationPolicy{})
Copy link
Contributor

Choose a reason for hiding this comment

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

NewGovPermissionKeeper should already do what SudoAuthorizationPolicy is doing

Copy link
Member Author

Choose a reason for hiding this comment

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

sweet, replaced this with NewGovPermissionKeeper

Copy link
Contributor

@nicolaslara nicolaslara left a comment

Choose a reason for hiding this comment

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

LGTM with the exception of the comments I left

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:app-wiring Changes to the app folder C:CLI C:x/tokenfactory V:state/compatible/backport State machine compatible PR, should be backported
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants