Revisit weave.Context

[ethanfrey]

Is your feature request related to a problem? Please describe.

We keep putting more info in the context and now with discussions to pass this down to the orm and even store layers, I think we need to revisit the design.

The original design used context.Context to hold arbitrary values which can be extended outside of the framework. Multiple packages exposing their own Authenticator and storing with private type keys is a powerful use of this abstraction and a good use (in my mind) of context.Context, much like many go web frameworks.

Storing essential info, such as the KVStore and the Tx in the context was rejected due to non-transparency. And I think that is a good think to keep them out.

We now have a set of information stored in Context, that is framework-level and not to be defined by any modules. If I pull those out into one struct it would be:

type BlockInfo struct {
  Header abci.Header
  CommitInfo weave.CommitInfo
  Height int64
  BlockTime time.Time
  ChainID string
  Logger log.Logger
}

However, as of now there is a whole selection of function in context.go for these items

Describe the solution you'd like

I would like a solution that makes it very clear what data is available. Maybe we can de-deplicate info like Header and BlockTime with helpers.

As a strawman proposal, I would suggest something like this, but with vastly better naming:

type Environment struct {
  Auth context.Context
  Block BlockInfo
}

We can update eg. Handler to be:

	Check(env Environment, store KVStore, tx Tx) (*CheckResult, error)
	Deliver(env Environment, store KVStore, tx Tx) (*DeliverResult, error)

(or possible s/tx Tx/msg Msg/ as well)

Then if we wish to pull some info into the orm for example, we could just pass in the BlockInfo, not a generic context. Or, better yet, just the attributed we care about.

type Heighter interface {
  GetHeight() int64
}

// asks for too much
Put(env Environment, db KVStore, key []byte, model Model) error

// what are we even passing in
Put(ctx Context, db KVStore, key []byte, model Model) error

// ah... we just need the height info.. we can still pass in a BlockInfo though
Put(h Heighter, db KVStore, key []byte, model Model) error

Describe alternatives you've considered

The current state with the all-powerful Context As mentioned, I like the extensibility when needed, but I think this leads to a lack of clarity many places.

I am very happy for other solutions that resolve this issue as well.

[ruseinov]

First thing I wanted to mention - we have some helpers that make sure a certain value is set, or they panic/return error. How do we deal with that in case of a struct that is supposed to have all these parameters? I guess we should revisit how this is being populated, so that we're guaranteed that parameters are always there when we need them, or we fail somewhere on framework-level.

I quite liked the signature with context and not context included in the environment, cause then the usage of context is not as transparent.

I would suggest to group all these properties in a struct and indeed use them where needed, maybe even by interface, but still keep them in a context. Or extend the signature to be
Check(ctx Context, info BlockInfo, store KVStore, tx Tx) (*CheckResult, error)

IMHO the context should be visible and not hidden in another struct.

[ethanfrey]

Check(ctx Context, info BlockInfo, store KVStore, tx Tx) (*CheckResult, error)

A bit longer function signature, but very clear. I like

[alpe]

The old weave.Context contained very different things at once:

• BlockInfo
• AuthN
• Logger
• Extension point to enrich context

Moving some of this into structured data makes sense for me. Ideally, I would like to see AuthN as structured data (weave.Conditions), too.
The logger should not be passed via context but as a constructor argument to the handlers/decorators.

Check(ctx Context, info BlockInfo, store KVStore, tx Tx) (*CheckResult, error) makes BlockInfo more prominent than needed. I think passing it via a new weaveContext type would be good enough.

[ethanfrey]

@alpe So you prefer some struct like

type Environment struct {
  Auth context.Context
  Block BlockInfo
}

over the second arg?

I like the idea of passing in the logger in the constructor, but we currently set info on it for every call:

	ctx := weave.WithLogInfo(b.BlockContext(),
		"call", "deliver_tx",
		"path", weave.GetPath(tx))

https://github.com/iov-one/weave/blob/master/app/base.go#L42-L45

I would actually like to add the height there, for nice structured logging. The only actual logging in current code is not in any Handlers, but in the generic LoggingDecorator:

	logger := weave.GetLogger(ctx).With("duration", delta/time.Microsecond)
	// ...
	logger.Info(msg)

https://github.com/iov-one/weave/blob/master/x/utils/logging.go

I could pass it in as a top-level argument to Deliver, but decided to hide it "somewhere". Better suggestions where to hide it are welcome.

[ethanfrey]

Also, I do not think BlockInfo (maybe renamed) is too prominent. It is central to the control flow of many modules - aswap, escrow, gov - and controls whether or not transations succeed.

The other essential info is h.auth.HasAddress(ctx), which puts the onus on the x.Authenticator implementations to handle the context, rather than making the "smart contracts" understand what we throw in the context (as now).

I think anything that is critical to the control flow should be explicit. You can see how cosmos-sdk throws everything into their Context mod:

func NewContext(ms MultiStore, header abci.Header, isCheckTx bool, logger log.Logger) Context {
	c := Context{
		Context: context.Background(),
		pst:     newThePast(),
		gen:     0,
	}

	c = c.WithMultiStore(ms)
	c = c.WithBlockHeader(header)
	c = c.WithChainID(header.ChainID)
	c = c.WithIsCheckTx(isCheckTx)
	c = c.WithTxBytes(nil)
	c = c.WithLogger(logger)
	c = c.WithVoteInfos(nil)
	c = c.WithGasMeter(stypes.NewInfiniteGasMeter())
	c = c.WithMinGasPrices(DecCoins{})
	c = c.WithConsensusParams(nil)
	c = c.WithEventManager(NewEventManager())

	return c
}

https://github.com/cosmos/cosmos-sdk/blob/master/types/context.go#L38-L58

Taking our current approach to it's logical conclusion. With such items as the reference to the database, as well as the gasMeter to abort transactions early, being stored in some opaque Context object, which is not actually context.Context, but a cosmos-specific "improvement"

This should highlight some of the motivation of simplifying into a much more standard Context with no "magic info" assumed to be in there. Nice discussions on #835 about possible new uses with such a Context

[alpe]

Thanks for your additional infos and comments. It is very good that we address this issue before it becomes a problem. I had some structured data context like this in mind:

type WeaveContext struct{
	context.Context  // transient unstructured data
	AuthN []Condition
	Block BlockInfo
	Logger Logger // can not be removed easily
}

[ethanfrey]

If we go this route, why not just Deliver(request Request) (*DeliverResult, error)?

And then a struct:

type Request struct {
  Auth []Address
  Block BlockInfo
  Ctx context.Context
  DB CacheableKVStore
  Logger Logger
  Tx Tx
}

I would rather collapse all args into one struct. Or all as separate args. I do see the value of this approach actually.