Proposal for BaseApp refactor: CommitMultiStore and Router as constructor args

[ebuchman]

BaseApp is a simple layer over ABCI whose primary jobs are to manage the DeliverTx and CheckTx states correctly and to bind our model of stores (ie. the MultiStore with capkeys for access) with our model of routing (ie. AnteHandler and Handler).

It might be clearer if we set up the CommitMultiStore and Router before creating the BaseApp, and pass them in as arguments to the BaseApp.

This would help clarify the role of these three objects and would make their exposed method sets easier to understand.

Part and parcel of this would be to put all ABCI msg handling on the router, eg. SetAnteHandler, SetInitChainer/BeginBlocker/EndBlocker.

We should probably make TxDecoder an argument to NewRouter since it's necessary. The rest can use SetXxx. And then there would be AddHandler

In conclusion, I propose:

• BaseApp only exposes ABCI methods (satisfies abci.Application)
• BaseApp takes Router and CommitMultiStore as arguments
• Router constructor takes TxDecoder. Router gets all SetXxx methods from BaseApp

This replaces #480 and subsumes #557

[rigelrozanski]

I'll think about this one a bit more - sounds like what your saying makes good sense.

[mossid]

Wouldn't it be better if we make a wrapper interface for CommitMultiStore in package baseapp? There are some functions like BaseApp.LastBlockHeight those depends on BaseApp.cms. We can move it to something like LastBlockHeight(CommitMultiStore) in helpers.go, but the form of cms.LastBlockHeight() is more consistent.

[ebuchman]

I wonder how much those functions really need to be exposed on the baseapp itself. Maybe its fine if folks call cms.LastBlockHeight() directly on the cms - they'll have access to it anyways if they're passing it into NewBaseApp

[rigelrozanski]

So under this organization how would init genesis be implemented... also any understanding as to why Jae may disagree with this as you implied?

Some thoughts:

BaseApp only exposes ABCI methods (satisfies abci.Application)

I like this I think makes for cleaner design

Router constructor takes TxDecoder. Router gets all SetXxx methods from BaseApp

Could you explain SetXxx in this context, a bit confused

[cwgoes]

Tabled per SDK design meeting to post-launch.

[jaekwon]

For the record, I disagree with a few points here. e.g. the router should handle msgs, nothing else. And BaseApp having a router auto-constructed is fine enough imo.

[ebuchman]

basically what I'm trying to get at is that the BaseApp is confounding two seemingly distinct things.

One is a core way to build ABCI apps with a particular managing of CheckTx/DeliverTx state and caching on a store.

The other is defining the actual execution details, eg. the SetBeginBlock etc., the Router, codespacer, and so on.

This was a proposal to better capture that distinction by passing in the ABCI concerns as a separate object, so that baseapp could in theory be used with different approaches to routing and begin/endblock and codespacing and so on.

Perhaps the confusion was over lumping the ABCI concerns into "Router". Would be better as an ABCIExecutor or something that itself contains a Router, at least in the main implementation.

Anyways, we can close it. Perhaps we'll revisit sometime down the road ...