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

solana-ibc: encode client state using borsh #44

Merged
merged 8 commits into from
Nov 1, 2023
Merged

solana-ibc: encode client state using borsh #44

merged 8 commits into from
Nov 1, 2023

Conversation

mina86
Copy link
Collaborator

@mina86 mina86 commented Oct 21, 2023

Prefer borsh over JSON since it’s more compact and faster. Since
tendermint’s ClientState doesn’t support borsh, use protobuf encoding
in the borsh format.

Note that protobuf encoding isn’t deterministic which means that the
borsh encoding for AnyClientState isn’t deterministic either. Having
said that, we should be good so long as clients wanting to verify the
value don’t re-encode the borsh-encoded value.

Lastly, it’s worth noting that alternatively to using borsh we could
use Any protobuf encoding directly. That would be marginally less
efficient (since the entire type URL needs to be stored rather than
one-byte tag) but it would reduce amount of code.

Prefer borsh over JSON since it’s more compact and faster.  Since
tendermint’s ClientState doesn’t support borsh, use protobuf encoding
in the borsh format.

Note that protobuf encoding isn’t deterministic which means that the
borsh encoding for AnyClientState isn’t deterministic either.  Having
said that, we should be good so long as clients wanting to verify the
value don’t re-encode the borsh-encoded value.

Lastly, it’s worth noting that alternatively to using borsh we could
use Any protobuf encoding directly.  That would be marginally less
efficient (since the entire type URL needs to be stored rather than
one-byte tag) but it would reduce amount of code.
@mina86 mina86 requested a review from dhruvja October 21, 2023 07:26
Copy link
Collaborator

@dhruvja dhruvja left a comment

Choose a reason for hiding this comment

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

LGTM

@mina86 mina86 marked this pull request as draft October 21, 2023 14:03
@mina86 mina86 marked this pull request as ready for review November 1, 2023 09:05
@mina86 mina86 requested a review from dhruvja November 1, 2023 09:05
@mina86 mina86 enabled auto-merge (squash) November 1, 2023 09:06
@mina86 mina86 merged commit 36255f6 into master Nov 1, 2023
4 checks passed
@mina86 mina86 deleted the mpn/c branch November 1, 2023 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants