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

light-clients: update revision number for cf-guest and cf-solana to 1 #392

Merged
merged 1 commit into from
Oct 15, 2024

Conversation

dhruvja
Copy link
Collaborator

@dhruvja dhruvja commented Sep 7, 2024

This is to increase compatibility with other libraries which fail to
properly decode proto3 message with missing revision number field
(missing fields should be decoded as zero in proto3).

@dhruvja dhruvja requested a review from mina86 September 7, 2024 07:20
@mina86
Copy link
Collaborator

mina86 commented Sep 7, 2024

I don’t follow, why do we need this?

@dhruvja
Copy link
Collaborator Author

dhruvja commented Sep 7, 2024

well we used revision number as 1 everywhere since ibc-go used to omit it if it is 0. In our case it doesnt really matter what we use a revision number but i think, using 1 everywhere seems like a good option to maintain backward compatibility.

@mina86
Copy link
Collaborator

mina86 commented Oct 15, 2024

Right, but my point is why are we changing the revision number? Why can’t it be zero?

@dhruvja
Copy link
Collaborator Author

dhruvja commented Oct 15, 2024

if we make it to 0, we wouldnt be able to establish a connection with cosmos since it omits the field if the value is 0.

@mina86
Copy link
Collaborator

mina86 commented Oct 15, 2024

Yes, that’s how protobuf works. In proto3 zero fields are not encoded but then when decoding zero is implied. So Cosmos should decode it as zero just fine.

@dhruvja
Copy link
Collaborator Author

dhruvja commented Oct 15, 2024

but apparently it doesnt, i am not sure why. When we were having revision height as 0, we were getting missing field issue on ibc-go.

Copy link
Collaborator

@mina86 mina86 left a comment

Choose a reason for hiding this comment

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

OK, though now the rollout is potentially tricky, no? This needs to be updated in the solana-ibc and on Picasso at the same time, no?

@dhruvja
Copy link
Collaborator Author

dhruvja commented Oct 15, 2024

well no, since picasso uses cf-guest from composable-ibc.

@mina86
Copy link
Collaborator

mina86 commented Oct 15, 2024

But that uses this cf-guest as dependency, no?

@dhruvja
Copy link
Collaborator Author

dhruvja commented Oct 15, 2024

the cf-guest light client on picasso doesnt implement ClientStateExecution trait so we wont have an issue over there.

@mina86
Copy link
Collaborator

mina86 commented Oct 15, 2024

LGTM

@dhruvja dhruvja merged commit 8ff95be into master Oct 15, 2024
4 checks passed
@dhruvja dhruvja deleted the update-revision-number branch October 15, 2024 19:31
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