fix: Add gas metering to BlockBeforeSend #6757
Conversation
|
devbot help |
x/tokenfactory/keeper/before_send.go
Outdated
|
|
||
| // consume gas used for calling contract to the parent ctx | ||
| ctx.GasMeter().ConsumeGas(childCtx.GasMeter().GasConsumed(), "track before send gas") | ||
| childCtx := ctx.WithGasMeter(sdk.NewGasMeter(types.TrackBeforeSendGasLimit)) |
There was a problem hiding this comment.
Hmm a 100k gas limit seems very low here -- even the example blacklist contract costs 150k for a send right?
There was a problem hiding this comment.
Mostly were random numbers for gas metering, do you have idea on how much it should be capped at?
There was a problem hiding this comment.
def agreed 100k sounds too low, probably should be like 500k for now
There was a problem hiding this comment.
This looks good as a patch, although with this change the distinction between BlockBeforeSend and TrackBeforeSend becomes obsolete and could be remerged into just BeforeSend.
Created an issue #6775 to track this – it's not necessarily urgent, but better to do it sooner since the API break will be more costly as more contracts onboard to using this feature
| childCtx := ctx.WithGasMeter(sdk.NewGasMeter(types.BeforeSendHookGasLimit)) | ||
| _, err = k.contractKeeper.Sudo(childCtx.WithEventManager(em), cwAddr, msgBz) | ||
| if err != nil { | ||
| return errorsmod.Wrapf(err, "failed to call before send hook for denom %s", coin.Denom) |
There was a problem hiding this comment.
nit: would be good to have custom error type here
Closes: #XXX
What is the purpose of the change
Adds gas metering to BlockBeforeSend.
Testing and Verifying
Add relevant test cases.
Documentation and Release Note
Unreleasedsection ofCHANGELOG.md?Where is the change documented?
x/{module}/README.md)