Cosmos-sdk: Error Codespacing

[jaekwon]

Goal

We want some logical code unit (modules/keepers/handlers or something) to not have to worry about de-duping codes at an ecosystem-scale. (though project-scale would be fine e.g. if it were done in a central place like app.go... but given the number of errors this is not ideal, so we need something more course in granularity.)

Premises

• Even if we were to register a human readable "name" for namespacing errors, there can still be conflicts, unless we tie the error code to the implementation itself (e.g. "github.com/tendermint/..."), which sounds like a bad idea for everyone. So, if we're going to solve this problem, we might as well solve it "correctly", by explicit registration/reservation of some "codespace" in app.go.
• There will be many codespaces, and for each codespace, we want to support many codes. 16 bits for each gives us 64,000 codespaces and 64,000 codes for each codespace. While 64,000 codes for a codespace might seem excessive, it may come in handy if the module wants to dynamically generate codes to a small degree.
• We don't necessarily want to require namespacing by module or otherwise make it too implementation specific, as error codes are primarily about user concerns, and should be implementation agnostic (e.g. ideally others should be able to implement the Cosmos SDK using alternative codebases/libraries/architecture).
• The global func Err*(...) sdk.Error constructors in */errors.go are really nice, but we want to preserve least-authority model. This means we have to pass in the codespace into the sdk.Error constructor somehow, or at least set it later after construction. (I recommend the former approach here).
• We don't want to use tags, as that's conflating two things... the error identity (which must be short for Tendermint generality, and thus is a fixed numerical type) and further details about the error. More specifically, the codespace+code SHOULD uniquely identify an internationalized message, or a message formatter in the client. If the message includes a formatter (e.g. "{{name}}"), the client SHOULD be able to format it using the tag system.
• We now have a canonical version of the Tracing error implemented in tmlibs/common (see ErrorWrap and NewError..., it was derived from cosmos-sdk's Error). We should use that somehow.
• Each module/keeper/handler MUST be able to import/use codespace/codes from other modules/keepers/handlers. For example, a gov transaction must be able to return a "invalid input" error with the same codespace as the coinKeeper, if it's going to have a reference to the coinKeeper, and gets an error from the coinKeeper's methods. (Least-authority kinda bleeds here, but we can fix this in the future by making the codespace a key as well, but lets not do this yet).

Observations

Further, some observations from the way we're using the SDK for staking/bank/ibc:

• There are 2 broad places where an sdk.Error gets returned... from ValidateBasic(), and from Keeper/Mapper functions.
• x/stake/handler.go has methods that return sdk.Error outside the context of a Keeper, but there's still a "primary" stake keeper.
• x/ibc/mapper.go has IBCMapper.PostIBCPacket(ctx,packet) sdk.Error and IBCMapper.ReceiveIBCPacket(ctx,packet) sdk.Error, but the mapper itself doesn't make use of the sdk.Error system yet.
• x/auth/mapper.go doesn't return sdk.Error at all, it just returns errors.

Proposal

Given these premises and observations, I suggest we adopt the following:

• By convention lets say that a Mapper doesn't return an sdk.Error. Mappers by convention will return normal "error" errors. Mapper methods can take other mappers as arguments, but not other Keepers. Keeper methods can take both as arguments. This makes the Mapper more of a "lower-level" abstraction, whereas Keepers are "high-level", or "closer" to the ABCI result where the code ultimately matters.
• Rename IBCMapper to IBCKeeper.
• sdk.Error.ABCICode() should be an ABCICodeType (new) which is uint32.
• sdk.Error.Code() should be a CodeType which is uint16 (currently uint32).
• sdk.Error.Codespace() should be CodespaceType (new) which is uint16.
• sdk.Error.ABCICode() is (Codespace << 16) | Code
• sdk.Router.AddRoute should take a Codespace as an argument, which applies both in BaseApp.runTx which uses the router to set the err Codespace upon ValidateBasic failure. The context should also be initialized with the codespace, e.g. app.checkState.ctx.WithTxBytes(txBytes).WithCodespace(codeSpace).
• Each handler can thus use ctx.GetCodespace() uint16 if it wanted to, even without any Keepers as arguments.
• If the Keeper returns an sdk.Error, the handler should ideally just return it, as the Keeper is also responsible for setting the namespace for convenience, and to override it is confusing.
• In app.go, we should reserve/register codespaces for each keeper, by using some object called a Codespacer.

Codespacer in app.go

// in app.go, after keepers are created:
spacer := app.GetCodespacer()
coinKeeper.ReserveCoinspace(spacer)             // default is 100, panics if 100 is already reserved.
stakeKeeper.ReserveCodespace(spacer)            // default is 200
govKeeper.ReserveCodespace(spacer)              // default is 300
govExtensionKeeper.ReserveCodespace(spacer)     // default is 310
ibcKeeper.ReserveCodespace(spacer)              // default is 400
someKeeper.ReserveCodespaceWith(spacer, 401)    // default is 400

// or may be more conveniently,
type CodespaceReserver interface {
  ReserveCodespace(Codespacer)
  ReserveCodespaceWith(Codespacer, Codespace)
}
spacer := app.GetCodespacer()
spacer.Register(coinKeeper, stakeKeeper, govKeeper, govExtensionKeeper, ibcKeeper,
  someKeeper, 401,
  otherKeeper, 402,
  ...)

these in turn call spacer.ReserveCodespace(codespace uint16) which panics and suggests the next largest codespace number available. It makes no sense to return an error, as it's programmer error, and we definitely don't want to encourage post-app-init dynamic codespacing.

In the example above, govExtensionKeeper is a separate keeper from govKeeper, but since it's in the same "concern/family" of governance, they're both in the 300~399 range. someKeeper's default codespace is 400 which conflicts with our own (perhaps it's an alternative IBC implementation.). So we need to override it with a custom codespace of 401. Declaring a codespace family to be 100 codespaces large means we can have 2^16/100 = 655 codespace families. Governance, staking, ibc, ... seems like 655 might be sufficient for 99% of use-cases. Within the 100 range, perhaps by convention the 10's are used for standard extensions, while the 1's are used for de-duping. (See 310 vs 401 above).

There's nothing forcing the SDK programmer to register these keepers, and nothing forcing Keepers to be CodespaceReservers. A good app.go just does so by convention. If the app doesn't do this, the worst case is that the error codespace is incorrect.

Error constructors

The Error constructors should look like the following:

func ErrInvalidInput(codespace uint16, msg string) sdk.Error {
    return newError(codespace, CodeInvalidInput, msg)
}
func ErrNoInputs(codespace uint16) sdk.Error {
    return newError(codespace, CodeInvalidInput, "")
}
...

Error implementation

type sdkError struct {
        codespace CodespaceType // uint16
    code      CodeType      // uint16
    err       cmn.Error
}

cmn.WrapError is cool in that it can prevent double-wrapping a cmn.Error, but (a) this wouldn't work well with sdk.Error wrapping a cmn.Error without some "system" that I can't think of off the top of my head if it's possible to do well at all, and (b) the SDK is creating a new ecosystem of libraries, so we can just enforce that Keepers pass on the sdk.Error rather than wrapping. Wrapping is necessary in the greater Go ecosystem because many interfaces require using the error return argument type. In SDK-land we can start using sdk.Error in the return argument type instead.

sdkError.err.Message() MUST return a human-readable message. It SHOULD be one internationalized/rendered message that can also be derived by the client from the codespace/code. If the error codespace/code is one that requires formatting, sdkError.err.Message() should contain the fully formatted message, and the sdk.Result should include tags (along with the uint32 ABCICodeType) such that the client can render the message itself. It's a "nondeterministic" field of ResponseDeliverTx, so ultimately it doesn't affect consensus. For now we should use this to render English response codes, and then move to client-side internationalization.

sdkError should expose tracing functionality on the unexposed cmn.Error field named err by calling err.TraceFrom. It should not expose T() or WithT(), as sdk.Error doesn't need to be "handled" by the SDK, but merely recorded on the chain. If some module/function happens to return a cmn.Error, since we can't override the cmn.Error's Message, the sdk.Error.err should wrap the cmn.Error as a "cause" of sdk.Error.err. See the comment about WithCauser here: https://github.com/tendermint/tmlibs/blob/develop/common/errors.go#L71 . This is an exception to the rule stated in that comment, so we'd want to use WithCauser to wrap any error when constructing sdk.Error.err.

[cwgoes]

I'm not clear on the intended distinction between Mappers and Keepers. https://github.com/cosmos/cosmos-sdk/pull/690 changed all modules to use Keeper, except for IBC, but only because IBC was in progress on another branch. The only remaining Mapper is the AccountMapper in x/auth.

According to notes from the last SDK design meeting:

Mapper vs Keeper
DECISION:  Call everything keeper for now
In theory, "mapper" is simple get/set, while keeper provides more functionality

Are user-written modules expected to use both Mappers and Keepers, with the error usage difference specified in the proposal above - if so, what functionality should users put in the Mapper, and what functionality should go in the Keeper?

IMO having two abstractions is unnecessary - if users write complex modules they can add subsidiary Mapper-like abstractions for lower-level functionality, but they're more likely to be module-specific data structures anyways (such as Pool in x/stake).

[jaekwon]

I like the idea that mappers are just simple getters and setters (or maybe some ORM features too in the future). I guess I'm also proposing that Mappers (by convention) not return sdk.Errors, but simple 'errors'.

Modules need not use Mappers at all. Keepers on the other hand... all handlers will in general have one primary keeper, but some can have more. Some handlers might have no keeper at all. Looks like we won't have may mappers in the beginning.

[cwgoes]

Are we agreed upon this rough model @sunnya97 @ebuchman? Can start implementation.

[cwgoes]

How do we associate codespaces with keepers?

The Context is passed through handler functions, and handler functions have a closure on referenced keepers, e.g. bank.CoinKeeper for any handler which modifies balances. The router can set the codespace on the Context when the handler is first called, but if the handler calls methods on the bank.CoinKeeper using that context, the wrong codespace will be returned for bank.CoinKeeper errors.

[cwgoes]

I'm not clear on what keeper.ReserveCodespace is intended to do. Should the keeper struct include a codespace, which is used for errors thrown by that keeper? In that case, why is there a Codespace() method on the Context object? Or is the Codespacer intended to somehow map keepers to codespaces, and set the codespaces when an error is handled in baseapp's logic?

[jaekwon]

The Context is passed through handler functions, and handler functions have a closure on referenced keepers, e.g. bank.CoinKeeper for any handler which modifies balances. The router can set the codespace on the Context when the handler is first called, but if the handler calls methods on the bank.CoinKeeper using that context, the wrong codespace will be returned for bank.CoinKeeper errors.

In that case, the codespace returned by errors returned from bank.CoinKeeper should just be passed through. It's only in the case that an error is generated from within the handler itself, that it would then use ctx.GetCodespace().

I'm not clear on what keeper.ReserveCodespace is intended to do. Should the keeper struct include a codespace, which is used for errors thrown by that keeper? In that case, why is there a Codespace() method on the Context object? Or is the Codespacer intended to somehow map keepers to codespaces, and set the codespaces when an error is handled in baseapp's logic?

Does this make sense?

type CodespaceReserver interface {
  ReserveCodespace(Codespacer)
  ReserveCodespaceAt(Codespacer, Codespace)
}

type Codespacer struct {...}
// Convenience registration chaining system.
func (_ *Codespacer) Register(reservers ...CodespaceReserver) *Codespacer {...}
// Convenience registration chaining system.
func (_ *Codespacer) RegisterAt(reserver CodespaceReserver, codespace uint16) *Codespacer {...}
// Actual reservation, panics if already reserved.
func (_ *Codespacer) Reserve(codespace uint16) {...}

spacer := app.GetCodespacer()
spacer.Register(coinKeeper, stakeKeeper, govKeeper, govExtensionKeeper, ibcKeeper).
  RegisterAt(someKeeper, 401).
  RegisterAt(otherKeeper, 402)

Maybe it makes sense to split Codespacer.

type CodespaceReserver interface {
  ReserveCodespace(*CodespaceTable)
  ReserveCodespaceAt(*CodespaceTable, Codespace)
}

type CodespaceTable struct {...}
func (_ *CodespaceTable) Reserve(codespace uint16) {...}

// Convenient registration interface
type CodespaceRegistrar struct {... cr CodespaceTable ... }
func (_ *CodespaceRegistrar) Register(reservers ...CodespaceReserver) *Codespacer {...}
func (_ *CodespaceRegistrar) RegisterAt(reserver CodespaceReserver, codespace uint16) *Codespacer {...}

[cwgoes]

In that case, the codespace returned by errors returned from bank.CoinKeeper should just be passed through. It's only in the case that an error is generated from within the handler itself, that it would then use ctx.GetCodespace().

Why would the handler function want to use a codespace provided by the context? NewHandler either takes a keeper or is a function on the keeper type; the handler will always have access to the keeper's codespace. Is there some situation in which the codespace should be dynamically set in the context rather than specified for the keeper?

I've implemented Codespacer as the following interface in https://github.com/cosmos/cosmos-sdk/pull/809/commits/ce61e660cf3472400bcbc63b24cfbef74658b5ec - examples/democoin/app/app.go:

// Generate codespacer
codespacer := sdk.NewCodespacer()

// Add handlers.
coinKeeper := bank.NewCoinKeeper(app.accountMapper)
coolKeeper := cool.NewKeeper(app.capKeyMainStore, coinKeeper, codespacer.RegisterDefault())
powKeeper := pow.NewKeeper(app.capKeyPowStore, pow.NewPowConfig("pow", int64(1)), coinKeeper, codespacer.RegisterDefault())
ibcMapper := ibc.NewIBCMapper(app.cdc, app.capKeyIBCStore, codespacer.RegisterDefault())
stakeKeeper := simplestake.NewKeeper(app.capKeyStakingStore, coinKeeper, codespacer.RegisterDefault())

Does that look reasonable? RegisterDefault just picks the next available codespace.

See also commentary - https://github.com/cosmos/cosmos-sdk/pull/809#issue-179962738.

[cwgoes]

Closed by https://github.com/cosmos/cosmos-sdk/pull/809.