Description

Closes: #XXXX

Updates the post handler to incorporate the sdk.Result and the success boolean. This enables use cases like refunding unused gas during failed txs.

type PostHandler func(ctx Context, tx Tx, res *Result, simulate, success bool) (newCtx Context, err error)

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.

I have...

[ ] included the correct type prefix in the PR title
[ ] added ! to the type prefix if API or client breaking change
[ ] targeted the correct branch (see PR Targeting)
[ ] provided a link to the relevant issue or specification
[ ] followed the guidelines for building modules
[ ] included the necessary unit and integration tests
[ ] added a changelog entry to CHANGELOG.md
[ ] included comments for documenting Go code
[ ] updated the relevant documentation or specification
[ ] reviewed "Files changed" and left comments if necessary
[ ] confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.

I have...

[ ] confirmed the correct type prefix in the PR title
[ ] confirmed ! in the type prefix if API or client breaking change
[ ] confirmed all author checklist items have been addressed
[ ] reviewed state machine logic
[ ] reviewed API design and naming
[ ] reviewed documentation is accurate
[ ] reviewed tests and test coverage
[ ] manually tested (if applicable)

Nov 21 '22 10:11 fedekunze

Codecov Report

Merging #13940 (99b69d5) into main (2739f83) will increase coverage by 0.00%. The diff coverage is 46.42%.

:exclamation: Current head 99b69d5 differs from pull request most recent head 1ded094. Consider uploading reports for the commit 1ded094 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #13940   +/-   ##
=======================================
  Coverage   56.25%   56.26%           
=======================================
  Files         667      664    -3     
  Lines       56576    56510   -66     
=======================================
- Hits        31829    31797   -32     
+ Misses      22165    22142   -23     
+ Partials     2582     2571   -11

Impacted Files	Coverage Δ
baseapp/baseapp.go	`76.76% <0.00%> (ø)`
testutil/mock/types_handler.go	`0.00% <0.00%> (ø)`
baseapp/options.go	`66.12% <100.00%> (ø)`
types/handler.go	`100.00% <100.00%> (ø)`
x/group/module/module.go	`48.05% <0.00%> (-0.10%)`	:arrow_down:
server/mock/tx.go	`53.33% <0.00%> (ø)`
x/auth/types/codec.go	`100.00% <0.00%> (ø)`
x/group/keeper/migrations.go
x/auth/types/credentials.go
x/group/migrations/v2/migrate.go
... and 3 more

Nov 21 '22 10:11 codecov[bot]

I don't want to encourage inspecting Msg results here as that could lead to skipping pathways like authz, gov, cosmwasm, ICA etc.

This is behavior is optional per app and doesn't need to be implemented on the SDK. This is to support full leftover gas refunds in case the transaction fails

What actual data is needed from Result to accomplish something like refunding gas?

@aaronc the leftover gas is included in the tx result. It can also be obtained from the ctx gas meter but in general inspecting the result can enable post-processing logic that is more customizable

Nov 21 '22 19:11 fedekunze

[Cosmos SDK] SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

36.8% Coverage
0.0% Duplication

Nov 21 '22 19:11 sonarqubecloud[bot]

@kocubinski I have addressed some of your the comments in https://github.com/cosmos/cosmos-sdk/pull/14261.

Dec 12 '22 15:12 Vvaradinov

Changes look reasonable to me, but there does seem to be contention on the signature and usage of post-handlers. Namely, should we encourage safety of post-handler usage by having it go through a router?

I would say if post-handlers can inspect message results then they should go through the same router as authz, gov, groups, etc. If they only are intended for tx level processing (like AnteHandlers), then no.

Dec 12 '22 18:12 aaronc

Well since a *Result in passed in, a handler could inspect. But why is inspection problematic?

Dec 12 '22 19:12 alexanderbez

Well since a *Result in passed in, a handler could inspect. But why is inspection problematic?

If it's used to conditionally do stuff based on Msg results that is bypassed in gov, group, authz, etc. execution. I think the solution is to either 1) provide an alternate way to do that and strongly advise against using this pathway for that or 2) don't exposed all of Result

Dec 12 '22 19:12 aaronc

@fedekunze what parts/fields of *Result do you need for your use-case(s)?

Dec 12 '22 21:12 alexanderbez

what parts/fields of *Result do you need for your use-case(s)?

@alexanderbez we need to be able to access the Result.MsgResponses in order to get the post msg state. We don't need to change the values from it, just read-only access

Dec 13 '22 08:12 fedekunze

what parts/fields of *Result do you need for your use-case(s)?

@alexanderbez we need to be able to access the Result.MsgResponses in order to get the post msg state. We don't need to change the values from it, just read-only access

What are you doing with these MsgResponses @fedekunze ? Would it be okay if the authz, gov, etc execution paths skipped this logic?

Dec 13 '22 16:12 aaronc

What are you doing with these MsgResponses?

@aaronc we need to use the field from the msg response to execute custom logic (eg: refunding leftover gas from EVM state transition, inspecting EVM logs, etc).

Would it be okay if the authz, gov, etc execution paths skipped this logic? If they only are intended for tx level processing (like AnteHandlers), then no.

The post handlers are intended to be in the same level as the AnteHandlers so I guess it's ok from our perspective to skip this logic.

If it's used to conditionally do stuff based on Msg results that is bypassed in gov, group, authz, etc. execution. I think the solution is to either 1) provide an alternate way to do that and strongly advise against using this pathway for that or 2) don't exposed all of Result

We technically only need MsgResponses so happy to update the PR to reflect that and use a []*Any field instead

Dec 14 '22 15:12 fedekunze

@SpicyLemon I saw your commit referencing this PR. Could you share with us what's the use case you're looking for with this new functionality so that the core team is aware?

Dec 15 '22 08:12 fedekunze

@SpicyLemon I saw your commit referencing this PR. Could you share with us what's the use case you're looking for with this new functionality so that the core team is aware?

My PR was an update to our fork to incorporate the v0.46.7 changes (we were previously based off of v0.46.6). One of the commits that was part of that had a message referencing this ticket: https://github.com/cosmos/cosmos-sdk/commit/25449b5812. That's why it got linked.

That being said, we do have an alteration to our fork that is related. In baseapp/baseapp.go, after the call to the post handler, we have this:

		var feeEvents sdk.Events
		if mode == runTxModeDeliver {
			// When block gas exceeds, it'll panic and won't commit the cached store.
			// this should be called before we charge additional fee( otherwise would
			// defy the whole point of charging additional fee at the end)
			consumeBlockGas()

			// apply fee logic calls
			feeEvents, err = FeeInvoke(mode, app, runMsgCtx)
			if err != nil {
				return gInfo, nil, nil, priority, ctx, err
			}
			// Only write the cache if FeeInvoke didn't return an error.
			msCache.Write()
			app.deliverState.eventHistory = append(app.deliverState.eventHistory, result.Events...)
		}

That FeeInovoke calls an injectable function. For us, it's one that charges extra fees based on the message types. We want that to happen after gas is consumed though, so that the gas is still consumed when not enough fees are provided to cover these extra fees plus the gas.

We've another injectable addition after that, that allows us to tweak the events. Our use case is to fix the fee entries when gas is consumed but the tx fails.