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.

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

AppendJson is a bit misleading #1531

Closed
liamsi opened this issue Jul 4, 2018 · 6 comments
Closed

AppendJson is a bit misleading #1531

liamsi opened this issue Jul 4, 2018 · 6 comments

Comments

@liamsi
Copy link
Contributor

liamsi commented Jul 4, 2018

While it sounds like it will append to the end, it uses a golang map internally. Hence, the passed key won't necessarily end up at the end (and the written JSON can have a completely different key ordering):

cosmos-sdk/server/util.go

Lines 126 to 136 in 270e216

// append a new json field to existing json message
func AppendJSON(cdc *wire.Codec, baseJSON []byte, key string, value json.RawMessage) (appended []byte, err error) {
var jsonMap map[string]json.RawMessage
err = cdc.UnmarshalJSON(baseJSON, &jsonMap)
if err != nil {
return nil, err
}
jsonMap[key] = value
bz, err := wire.MarshalJSONIndent(cdc, jsonMap)
return json.RawMessage(bz), err
}

This might causes some confusion. Suggestions: either InsertKeyAndSortJSON (sort keys after including the new key) , or, IncludeJSON (keeps the introduced non-determinism but is clearly named).

See also:
tendermint/go-amino#171 (comment)
#983 (comment)

@alexanderbez
Copy link
Contributor

Ahhh confusing indeed! I think we could have both solutions you suggested actually and leave it up to the client to choose which it wants 👍

@alexanderbez
Copy link
Contributor

I'll address this in #983 :-)

@liamsi
Copy link
Contributor Author

liamsi commented Jul 5, 2018

Just to make sure we won't do double the work: I'm currently adding a method that takes arbitary JSON and returns (semantically) the same JSON but sorted by keys.

@alexanderbez
Copy link
Contributor

@liamsi perfect, I won't implement the sorted variant, I'll just rename the existing function. Also, fun fact, json.Marshal already sorts by keys, but I'm not sure how amino does it.

@liamsi
Copy link
Contributor Author

liamsi commented Jul 5, 2018

I know, my idea was to take arbitrary json, decode into an interface{} and call json.Marshal. I still need to do some tests though: https://github.com/cosmos/cosmos-sdk/compare/develop...Liamsi:sorted_json?expand=1#diff-44da3d359afd2823a31e4fb048bdfdf4R142 (also we don't want the json to be indented by default)

Amino doesn't by key (like proto3 doesn't sort field names either; although map keys are sorted by proto3 IIRC).

@alexanderbez
Copy link
Contributor

Closed via #1559

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants