Update Handler Interface & Improve Error Usage #5072

alexanderbez · 2019-09-19T19:57:24Z

closes: #4844

/cc @ethanfrey @marbar3778

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (docs/)
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the github PR explorer

For Admin Use:

Added appropriate labels to PR (ex. wip, ready-for-review, docs)
Reviewers Assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

alexanderbez · 2019-09-20T14:02:36Z

Blocked by #5006. We should wait till that's finalized and merged as both modify BaseApp.

ethanfrey

Nice start, and this does bring up some issues on the interfaces. Especially with GasHandling. After some reflection on the code base, I start to believe that GasUsed/GasWanted don't belong in Result after all. This would allow a proper *Result, err to be X, nil or nil, X as long as we pass the gas info some other way.

My reasoning is the following:

Result type is focused on defining the information a Handler returns. Not on the needs of baseapp.
The actual result values for GasWanted comes from the anteHandler and is not read from the Handler
While runMsg does set GasUsed, it is always overridden in the last defer of runTx: https://github.com/cosmos/cosmos-sdk/blob/d5ec8427e2254fdf6e4ceec1f7ad9503b04646f7/baseapp/baseapp.go#L548-L550

In fact, a Handler can return any info in these two Gas* fields and they will be ignored. So why expose it to them?

If we look at the end result in the abci interface:

	return abci.ResponseDeliverTx{
		Code:      uint32(result.Code),
		Codespace: string(result.Codespace),
		Data:      result.Data,
		Log:       result.Log,
		GasWanted: int64(result.GasWanted), // TODO: Should type accept unsigned ints?
		GasUsed:   int64(result.GasUsed),   // TODO: Should type accept unsigned ints?
		Events:    result.Events.ToABCIEvents(),
	}

https://github.com/cosmos/cosmos-sdk/blob/d5ec8427e2254fdf6e4ceec1f7ad9503b04646f7/baseapp/abci.go#L192-L201

I see this info coming from various sources:

Code/Codespace/Log represent info from error, as you are refactoring now
Data/Log/Events represent info from Result, set on successful execution
GasUsed/GasWanted is metering info which is handled in the runTx wrapper and not in a lower-level component. I would add a separate type for this.

My suggestion is:

Leave runMsg with (*Result, error) return value, and set Result to nil on error
Update runTx return value to (GasInfo, *Result, error) where Result and error are as runMsg (plus some other cases), but GasInfo is always set (from the defer function).

type GasInfo struct {
	GasWanted uint64
	GasUsed uint64
}

This actually would leave a super-minimal Result type, which is actually the only info a Handler can return on success:

type Result struct {
  Log string
  Data []byte
  Events Events
}

What do you think? I know it adds a type, but it can be a private type and is only used to communicate between runTx and CheckTx/DeliverTx, while further simplifying the Result interface to the essential. Since much more development work is expected to happen producing implementations of the Handler interface, than in the BaseApp wrapper around them, it seems like a nice tradeoff to me to make more simplicity for the Handler developers, which is actually the main goal of separating out the error return value, to make error handling simpler (and with better logging).

ethanfrey · 2019-09-20T22:01:00Z

types/tx_msg.go

@@ -17,7 +17,7 @@ type Msg interface {

 	// ValidateBasic does a simple validation check that
 	// doesn't require access to any other information.
-	ValidateBasic() Error


Error is a superset of error, so all existing code will work.
But they will have to update their function signatures, right?

I believe so

ethanfrey · 2019-09-20T22:03:24Z

types/handler.go

-type Handler func(ctx Context, msg Msg) Result
+// A reference to a Result type must be returned upon success and an error upon
+// failure.
+type Handler func(ctx Context, msg Msg) (*Result, error)


Okay, I would actually wonder if we want to give a grace period for people to upgrade their 3rd party modules.
Otherwise this PR will leave the entire SDK in a broken state for a while, no?

Like call the new Handler MsgHandler and then have a wrapper function that converts Handler -> MsgHandler

Otherwise, there is quite some work to clean up code before this can go into master.
With the incremental approach, you can split that into several prs (even if all done before a release)

This seems more plausible approach as well.

ethanfrey · 2019-09-20T22:03:55Z

types/errors/errors.go

@@ -126,7 +126,7 @@ func ABCIError(codespace string, code uint32, log string) error {

 // Error represents a root error.
 //
-// Weave framework is using root error to categorize issues. Each instance
+// The SDK uses root errors (see above) to categorize issues. Each instance


Thanks, missed this... as you see much of this code was copied, with minor adaptations (like adding codespaces)

ethanfrey · 2019-09-20T22:04:32Z

types/errors.go

@@ -186,7 +195,7 @@ type Error interface {
 }

 // NewError - create an error.
-func NewError(codespace CodespaceType, code CodeType, format string, args ...interface{}) Error {
+func NewErrorz(codespace CodespaceType, code CodeType, format string, args ...interface{}) Error {


Why add the z?
If intentional, I guess the godoc should be updated also

This seems more like a typo?

This is intentional; I wanted the go compiler that resulted in this to help updating code due to new APIs. This PR is still a DRAFT; ignore.

ethanfrey · 2019-09-20T22:05:35Z

baseapp/baseapp.go

@@ -522,7 +523,7 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte, tx sdk.Tx) (result sdk

 	// only run the tx if there is block gas remaining
 	if mode == runTxModeDeliver && ctx.BlockGasMeter().IsOutOfGas() {
-		return sdk.ErrOutOfGas("no block gas left to run tx").Result()
+		return nil, sdkerrors.Wrap(sdkerrors.ErrOutOfGas, "no block gas left to run tx")


Yes, looks good to me. I hope this format looks good for you also.

ethanfrey · 2019-09-20T22:06:28Z

baseapp/baseapp.go

 	}

 	// only update state if all messages pass
-	if result.IsOK() {
+	if err != nil {


this should be if err == nil

actually can be combined with the above statment...
if err == nil && mode != runTxDeliver { ...Write() }
return result, err

ethanfrey · 2019-09-20T22:28:30Z

baseapp/baseapp.go

 				log := fmt.Sprintf("recovered: %v\nstack:\n%v", r, string(debug.Stack()))
-				result = sdk.ErrInternal(log).Result()
+				err = sdkerrors.Wrap(sdkerrors.ErrPanic, log)


nice use of ErrPanic

ethanfrey · 2019-09-20T22:31:08Z

baseapp/baseapp.go

@@ -610,79 +615,68 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte, tx sdk.Tx) (result sdk
 	// MultiStore in case message processing fails. At this point, the MultiStore
 	// is doubly cached-wrapped.
 	runMsgCtx, msCache := app.cacheTxContext(ctx, txBytes)
-	result = app.runMsgs(runMsgCtx, msgs, mode)
+	result, err = app.runMsgs(runMsgCtx, msgs, mode)


Okay, so the reason we need result even if there is an error is to capture the gas consumed, right?

Even if the data, events, etc of result are meaningless in error, the gas usage still needs to be communicated, as that is handled above? (Trying to understand the flow)

ethanfrey · 2019-09-20T22:32:12Z

baseapp/baseapp.go

-		// skip actual execution for CheckTx mode
-		if mode != runTxModeCheck {
-			msgResult = handler(ctx, msg)
+			err = sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "unrecognized message type: %s,", msgRoute)


Why not return nil, err, which would be closest to the old behavior?

ethanfrey · 2019-09-20T22:35:11Z

baseapp/baseapp.go

 	if err := validateBasicTxMsgs(msgs); err != nil {
-		return err.Result()
+		return nil, err


This will panic... since the first defer block above sets result.GasWanted = gasWanted, we can never safely return nil here.

I didn't realize this case, and it seems this function must either always return a valid Result (in which case make it Result, not *Result so at least the empty value is valid), or the logic on 548-549 needs to change (I am not sure how that has repercussions on the rest of the stack)

Update Handler interface

10ee7bf

alexanderbez added C:baseapp WIP T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). labels Sep 19, 2019

More updates

1dd4f46

alexanderbez mentioned this pull request Sep 19, 2019

Improve Handler interface #4844

Closed

4 tasks

alexanderbez added 6 commits September 20, 2019 09:07

Cleanup godoc in errors

b37ae5c

Comment out Code and Codespace from result

c7a2038

Refactor runMsgs

8665132

Add sdkerrors package

0f21fed

Update Tx and Msg interfaces

4528f24

Update runTx

7cbb918

alexanderbez added the S:blocked Status: Blocked label Sep 20, 2019

ethanfrey reviewed Sep 21, 2019

View reviewed changes

tac0turtle removed the S:blocked Status: Blocked label Oct 16, 2019

alexanderbez closed this Dec 17, 2019

alexanderbez deleted the bez/4844-handler-error-refactor branch December 17, 2019 19:08

alexanderbez mentioned this pull request Dec 17, 2019

Refactor Error Handling #5421

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Handler Interface & Improve Error Usage #5072

Update Handler Interface & Improve Error Usage #5072

alexanderbez commented Sep 19, 2019 •

edited

Loading

alexanderbez commented Sep 20, 2019

ethanfrey left a comment

ethanfrey Sep 20, 2019

tnachen Sep 22, 2019

ethanfrey Sep 20, 2019

tnachen Sep 22, 2019

ethanfrey Sep 20, 2019

ethanfrey Sep 20, 2019

tnachen Sep 22, 2019

alexanderbez Sep 22, 2019

ethanfrey Sep 20, 2019

ethanfrey Sep 20, 2019

ethanfrey Sep 20, 2019

ethanfrey Sep 20, 2019

ethanfrey Sep 20, 2019

ethanfrey Sep 20, 2019

ethanfrey Sep 20, 2019

Update Handler Interface & Improve Error Usage #5072

Update Handler Interface & Improve Error Usage #5072

Conversation

alexanderbez commented Sep 19, 2019 • edited Loading

alexanderbez commented Sep 20, 2019

ethanfrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez commented Sep 19, 2019 •

edited

Loading