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

Hash should be base64 encoded in JSON #2476

Closed
matevz opened this issue Dec 16, 2019 · 6 comments · Fixed by #2480
Closed

Hash should be base64 encoded in JSON #2476

matevz opened this issue Dec 16, 2019 · 6 comments · Fixed by #2480
Assignees
Labels
c:bug Category: bug c:common Category: common libraries p:0 Priority: High! bugs, address immediately s:good first issue Status: good for newcomers

Comments

@matevz
Copy link
Member

matevz commented Dec 16, 2019

Export hash.Hash to JSON as string.

Currently, it's exported as array of bytes, for example
"state_root":[217,195,97,253,63,21,161,178,145,7,97,119,244,3,68,229,186,166,212,28,194,165,17,0,134,211,10,91,142,3,135,115]
It should be exported as a base64 encoded string, like "state_root":"jycFqjlMlRtaL9QhhK9gqVezw4Lp4Kro34MSH4YggS8="

@kostko
Copy link
Member

kostko commented Dec 16, 2019

This is probably the result of switching the codec library with the built-in one which doesn't use Base64 by default for types that implement BinaryMarshaler. We probably need to go around and add TextMarshaler implementations to our types.

@kostko kostko added c:common Category: common libraries s:good first issue Status: good for newcomers labels Dec 16, 2019
@Yawning Yawning added c:bug Category: bug p:0 Priority: High! bugs, address immediately labels Dec 16, 2019
@Yawning
Copy link
Contributor

Yawning commented Dec 16, 2019

Increasing the priority of this because, the export format should be concrete sooner rather than later.

@matevz
Copy link
Member Author

matevz commented Dec 17, 2019

Do we also need to fix rust implementation anywhere?

@Yawning
Copy link
Contributor

Yawning commented Dec 17, 2019

Do we also need to fix rust implementation anywhere?

If the rust code uses something that isn't CBOR, then yes, but as far as I am aware, this is not the case.

@matevz
Copy link
Member Author

matevz commented Dec 17, 2019

This is used by genesis-playback for generating genesis_dump.json: https://github.com/oasislabs/oasis-core/blob/master/runtime/src/common/crypto/hash.rs#L6

@matevz
Copy link
Member Author

matevz commented Dec 20, 2019

This is now also correctly implemented in rust as part of #2462

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c:bug Category: bug c:common Category: common libraries p:0 Priority: High! bugs, address immediately s:good first issue Status: good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants