-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Error Codespacing #766
Comments
I'm not clear on the intended distinction between According to notes from the last SDK design meeting:
Are user-written modules expected to use both IMO having two abstractions is unnecessary - if users write complex modules they can add subsidiary |
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. |
How do we associate codespaces with keepers? The |
I'm not clear on what |
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().
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 {...} |
Why would the handler function want to use a codespace provided by the context? I've implemented // 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? See also commentary - #809 (comment). |
Closed by #809. |
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
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).Observations
Further, some observations from the way we're using the SDK for staking/bank/ibc:
IBCMapper.PostIBCPacket(ctx,packet) sdk.Error
andIBCMapper.ReceiveIBCPacket(ctx,packet) sdk.Error
, but the mapper itself doesn't make use of the sdk.Error system yet.Proposal
Given these premises and observations, I suggest we adopt the following:
(Codespace << 16) | Code
app.checkState.ctx.WithTxBytes(txBytes).WithCodespace(codeSpace)
.ctx.GetCodespace() uint16
if it wanted to, even without any Keepers as arguments.Codespacer in app.go
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:
Error implementation
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 usingsdk.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 exposeT()
orWithT()
, 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.The text was updated successfully, but these errors were encountered: