Conversation
Codecov Report
@@ Coverage Diff @@
## develop #3468 +/- ##
===========================================
+ Coverage 59.51% 59.97% +0.46%
===========================================
Files 135 135
Lines 9981 9960 -21
===========================================
+ Hits 5940 5974 +34
+ Misses 3669 3619 -50
+ Partials 372 367 -5 |
|
🔥🚀🌔 |
| // Delegates to CommitMultiStore if it implements Queryable | ||
| // Query implements the ABCI interface. It delegates to CommitMultiStore if it | ||
| // implements Queryable. | ||
| func (app *BaseApp) Query(req abci.RequestQuery) (res abci.ResponseQuery) { |
There was a problem hiding this comment.
Would be really nice to have the ABCI query functionality documented in detail for each module and query. cc @mossid
alexanderbez
changed the title
cwgoes
left a comment
cwgoes
left a comment
There was a problem hiding this comment.
These testcases look fine; thanks for cleaning up all the comments - can we also add some for:
LoadVersionusage- Bad query paths on the query router
- Quick message router tests (add
router_test.go)
And can we cleanup:
baseapp/helpers.go-RunForeveris not used, probably the other functions aren't either- Delete
baseapp/query.go- unused, very out of date - Remove the entire
testdatafolder - unused, very out of date
baseapp/baseapp.go
Outdated
| // Router returns the router of the BaseApp. | ||
| func (app *BaseApp) Router() Router { | ||
| if app.sealed { | ||
| // TODO: Why are panicing here on returning the router??? |
There was a problem hiding this comment.
Becase AddRoute can be called, I think
There was a problem hiding this comment.
is this right? can we update the comment?
| // Mount IAVL or DB stores to the provided keys in the BaseApp multistore | ||
| // MountStores mounts all IAVL or DB stores to the provided keys in the BaseApp | ||
| // multistore. | ||
| func (app *BaseApp) MountStores(keys ...*sdk.KVStoreKey) { |
There was a problem hiding this comment.
Can we make this MountStores function take both transient keys and store keys? It could look like:
func (app *BaseApp) MountStores(keys ...interface{}) {
for _, key := range keys {
switch key.(type) {
case *sdk.KVStoreKey:
if !app.fauxMerkleMode {
app.MountStore(key, sdk.StoreTypeIAVL)
} else {
app.MountStore(key, sdk.StoreTypeDB)
}
case *sdk.TransientStoreKey:
app.MountStore(key, sdk.StoreTypeTransient
}
default:
panic("Invalid key type")
}
}There was a problem hiding this comment.
I think this is a good idea - but perhaps best for a separate PR?
There was a problem hiding this comment.
Yeah. I think this falls under general app.go cleanup
|
@cwgoes I've address your comments. In addition, I also cleaned up the existing router a bit (now using a map instead of a slice). |
cwgoes
left a comment
cwgoes
left a comment
There was a problem hiding this comment.
A few further notes on comments; otherwise LGTM.
baseapp/baseapp.go
Outdated
| // Router returns the router of the BaseApp. | ||
| func (app *BaseApp) Router() Router { | ||
| if app.sealed { | ||
| // TODO: Why are panicing here on returning the router??? |
There was a problem hiding this comment.
is this right? can we update the comment?
|
@cwgoes addressed |
4 participants
Adds a test case for #3259 and a few other small test cases to increase coverage. In addition, minor cleanup to godocs and general structure (no functional changes). This PR is pending further additional test cases we deem necessary.
closes: #556
closes: #3259
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 entries in
PENDING.mdwith issue #rereviewed
Files changedin the github PR explorerFor Admin Use: