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

Remove AllowUpdateAfterMisbehaviour/AllowUpdateAfterExpiry from 07-tendermint client state. #5750

Open
2 of 3 tasks
DimitrisJim opened this issue Jan 26, 2024 · 6 comments
Open
2 of 3 tasks
Assignees
Labels
deprecated Issues to remove deprecated code icebox Issues that we will not address for the time being needs discussion Issues that need discussion before they can be worked on

Comments

@DimitrisJim
Copy link
Contributor

These have been marked as deprecated after 02-client refactor reference ADR. We could probably remove these in a fully compatible way by using [reserved}(https://protobuf.dev/programming-guides/proto3/#reserved) values.

Opening issue to keep track and discuss when to remove.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@DimitrisJim DimitrisJim added needs discussion Issues that need discussion before they can be worked on deprecated Issues to remove deprecated code labels Jan 26, 2024
@expertdicer
Copy link
Contributor

expertdicer commented Feb 19, 2024

Sorry for doing the issue before asking, but I've already done it.

@DimitrisJim
Copy link
Contributor Author

better late than never to drop a comment @expertdicer 😄! I see the PR is failing on e2es but don't have much time this month to see why (reserved might not be right approach?). I'll prioritize reviewing this when I get some additional time. Thanks!

@expertdicer
Copy link
Contributor

I'm still trying to debug the err, still don't know why 😅

@colin-axner
Copy link
Contributor

It might be possible that the counterparty has set these fields to true which means in the connection handshake, our code needs to be able to populate these fields when decoding. Will the reserved field work correct in this case @DimitrisJim?

@colin-axner
Copy link
Contributor

I looked into this a little. The SDK will fail to unmarshal unknown fields so when we mark existing fields as reserved the compiler sees these as unknown fields. We would need to require relayers to remove the fields from their grpc requests. Personally, I think it might be better to ice box this issue and wait until we restructure the client state for other reasons as well. What do you think @DimitrisJim?

@DimitrisJim
Copy link
Contributor Author

sgtm! this seems quite unfortunate though, iirc reserved was supposed to be a clean way to remove certain field from the proto structures.

@DimitrisJim DimitrisJim added the icebox Issues that we will not address for the time being label May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecated Issues to remove deprecated code icebox Issues that we will not address for the time being needs discussion Issues that need discussion before they can be worked on
Projects
Status: No status
Development

Successfully merging a pull request may close this issue.

3 participants