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

Change counterpartyInfo to channel #100

Merged
merged 17 commits into from
Dec 2, 2024
Merged

Conversation

AdityaSripal
Copy link
Member

Description

This PR renames counterpartyInfo to channel

Moreover it does not assume that the counterpartyId is necessarily a clientID. I think here the logic looks much more in-line with IBC-go and hopefully will do a lot to remove the confusions raised in the referenced spec issue.

For now, I have a single contract implementing ICS02 and ICS04 specs since ICS04 doesn't have too many additional functions. I can rename the file to CoreRouter to make this clearer rather than ICS02Client.

TODO:

  • Rename ICS02Client to something that indicates it is doing both ICS02 client and ICS04 channel
  • Fix e2e tests

ref: cosmos/ibc#1164


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Linked to GitHub issue with discussion and accepted design, OR link to spec that describes this work.
  • Wrote unit and integration tests.
  • Added relevant natspec and godoc comments.
  • Provide a conventional commit message to follow the repository standards.
  • Re-reviewed Files changed in the GitHub PR explorer.
  • Review SonarCloud Report in the comment section below once CI passes.

justfile Outdated Show resolved Hide resolved
@AdityaSripal AdityaSripal marked this pull request as ready for review November 19, 2024 15:03
@AdityaSripal AdityaSripal changed the title DRAFT: Change counterpartyInfo to channel Change counterpartyInfo to channel Nov 19, 2024
Copy link
Member

@srdtrk srdtrk left a comment

Choose a reason for hiding this comment

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

Good start. I still feel like it was easier to wrap your head around when it was called counterparty info. If we were explaining this to a 3rd party, the first one sounds simpler imo:

  • Each light client has a counterparty chain info associated with it.
  • Each light client has a channel associated with it that carries some counterparty info where channelId equals clientId.

But if we need to do this to follow spec then I won't block it. There are a couple more things to do before we can merge this:

  • Fix linting issues (just lint)
  • Have foundry tests all pass and codecov increase. (just test-foundry)
  • Regenerate abi (Probably generate-abi recepe in the justfile needs to be updated due to contract name change)
  • Update e2e tests
  • Update benchmarks

We can take care of the last three after you take the first two?

src/ICS26Router.sol Outdated Show resolved Hide resolved
src/ICSCore.sol Outdated Show resolved Hide resolved
src/ICSCore.sol Outdated
mapping(string clientId => CounterpartyInfo info) private counterpartyInfos;
contract ICSCore is IICS02Client, IICS04Channel, IICS02ClientErrors, Ownable {
/// @dev channelId => counterpartyInfo
mapping(string channelId => Channel channel) private channels;
Copy link
Member

Choose a reason for hiding this comment

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

CounterpartyInfo feels more descriptive ngl 😅. Because the same light client cannot have multiple channels on top in our implementation I think

interface IICS04ChannelMsgs {
/// @notice Channel information.
/// @custom:spec
/// https://github.com/cosmos/ibc/blob/67fe813f7e4ec603a7c5dec35bc654f3b012afda/spec/micro/README.md?plain=1#L91
Copy link
Member

Choose a reason for hiding this comment

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

The link should be updated?

Copy link

codecov bot commented Dec 2, 2024

Codecov Report

Attention: Patch coverage is 95.45455% with 1 line in your changes missing coverage. Please review.

Project coverage is 96.84%. Comparing base (169400e) to head (21d7beb).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
contracts/ICS26Router.sol 91.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #100      +/-   ##
==========================================
+ Coverage   95.33%   96.84%   +1.51%     
==========================================
  Files          11       11              
  Lines         450      539      +89     
==========================================
+ Hits          429      522      +93     
+ Misses         21       17       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@srdtrk srdtrk merged commit ee8cdff into main Dec 2, 2024
40 checks passed
@srdtrk srdtrk deleted the aditya/counterparty-to-channel branch December 2, 2024 10:03
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.

3 participants