-
Notifications
You must be signed in to change notification settings - Fork 18
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
feat: init custom fee antehandler and posthandler #27
Conversation
if tc.ExpPass { | ||
require.NoError(t, txErr) | ||
require.NoError(t, handleErr) | ||
require.NotNil(t, newCtx) | ||
|
||
s.Ctx = newCtx | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to do any validation on the sent amount? I know that this is technically going to be testing external code but ideally we check fee amount was deducted, and that the context priority was correctly set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So right now, all of these tests use a mock bank keeper, so the fees aren't really "sent". I figured we would do the validation of that in an e2e test, but we could also do it here.
wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea, im fine with e2e.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left some comments
Co-authored-by: David Terpay <[email protected]>
Co-authored-by: David Terpay <[email protected]>
Co-authored-by: David Terpay <[email protected]>
x/feemarket/post/fee.go
Outdated
return ctx, errorsmod.Wrapf(err, "unable to get fee market state") | ||
} | ||
|
||
state.Window[state.Index] += gas |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use state.Update
since that has a safety check on block gas limit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blessed be thee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FeeMarketKeeper
interfaceapp.go
ante
pkgante
andpost
packagesante
andpost