Skip to content
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.

Sign up for GitHub

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

simapp: use tmjson on InitChainer #7514

Merged
merged 2 commits into from
Oct 12, 2020
Merged

simapp: use tmjson on InitChainer #7514

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f5783f6
simapp: use tmjson on InitChainer
fedekunze Oct 12, 2020
f5775a0
Merge branch 'master' into fedekunze/tmjson
Oct 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion simapp/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"github.com/gorilla/mux"
"github.com/rakyll/statik/fs"
abci "github.com/tendermint/tendermint/abci/types"
tmjson "github.com/tendermint/tendermint/libs/json"
"github.com/tendermint/tendermint/libs/log"
tmos "github.com/tendermint/tendermint/libs/os"
tmproto "github.com/tendermint/tendermint/proto/tendermint/types"
Expand Down Expand Up @@ -450,7 +451,9 @@ func (app *SimApp) EndBlocker(ctx sdk.Context, req abci.RequestEndBlock) abci.Re
// InitChainer application update at chain initialization
func (app *SimApp) InitChainer(ctx sdk.Context, req abci.RequestInitChain) abci.ResponseInitChain {
var genesisState GenesisState
app.cdc.MustUnmarshalJSON(req.AppStateBytes, &genesisState)
if err := tmjson.Unmarshal(req.AppStateBytes, &genesisState); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a pity that the library does not provide a MustUnmarshal() method :(

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see it as an advantage. Panics should be avoided.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the reason for using tmjson here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought GenesisDoc was marshalled using tmjson on the TM side?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah wait, i might have read too fast, this is our own GenesisState. So it should be decoded with appCodec.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proper error handling:

  • define critical error type
  • pass it up with a stack
  • handle it in the main routine allowing to shutdown things (maybe some shutdown's are not relevant here, but this is the right approach anyway).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pass it up with a stack

I +1 this idea, and would even propose more aggressively to only allow Tendermint to panic. It's probably a larger discussion, but if the TM spec would expose some well-defined errors that cause TM to panic, then the SDK (or any state machine) should only return these errors, and contain 0 panic itself.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two weeks ago I've created an issue to clean the panics wherever possible: #7409

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably a larger discussion

I agree very much indeed to that. But until we got stuff sorted in Tendermint, there is not much we can do other than panic()ing in the SDK.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We generally have been using stdlib encoding/json

panic(err)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouch, can we return an error and handle it somewhere higher, please?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This must panic (it used to panic too). An error that happens in InitChain() is unrecoverable, as such it must cause the program to crash and make as much noise as possible.

}
return app.mm.InitGenesis(ctx, app.appCodec, genesisState)
}

Expand Down