ibc-go icon indicating copy to clipboard operation
ibc-go copied to clipboard

Remove `AllowUpdateAfterMisbehaviour`/`AllowUpdateAfterExpiry` from 07-tendermint client state.

Open DimitrisJim opened this issue 1 year ago • 6 comments

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

  • [x] Not duplicate issue
  • [x] Appropriate labels applied
  • [ ] Appropriate contributors tagged/assigned

DimitrisJim avatar Jan 26 '24 06:01 DimitrisJim

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

expertdicer avatar Feb 19 '24 06:02 expertdicer

better late than never to drop a comment @expertdicer :smile:! 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!

DimitrisJim avatar Feb 20 '24 11:02 DimitrisJim

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

expertdicer avatar Feb 22 '24 03:02 expertdicer

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 avatar Feb 27 '24 14:02 colin-axner

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?

colin-axner avatar May 22 '24 08:05 colin-axner

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

DimitrisJim avatar May 22 '24 09:05 DimitrisJim