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

Alias new channel IDs to the client IDs #6975

Closed
Tracked by #7134
AdityaSripal opened this issue Jul 29, 2024 · 8 comments
Closed
Tracked by #7134

Alias new channel IDs to the client IDs #6975

AdityaSripal opened this issue Jul 29, 2024 · 8 comments
Assignees
Labels
type: feature New features, sub-features or integrations
Milestone

Comments

@AdityaSripal
Copy link
Member

Allow multiple aliases to map to the same client-id. This should be implemented as a many-to-one layer in the channel keeper. When doing packet processing, retrieve the client id by resolving the channel id to the underlying client using the alias map.

The alias map can be added to using genesis, migration, or through governance.

This will allow a backwards compatible transition from ibc-classic to ibc-eureka since we do not need to migrate over channel ids and transfer escrows. It will also allow governance to use this alias map in the future for more readable channel-ids.

@AdityaSripal AdityaSripal added this to the Eureka alpha milestone Jul 29, 2024
@DimitrisJim
Copy link
Contributor

dropping comment made to keep track of additional validation func that can be used for counterparty id #6982 (comment)

@AdityaSripal AdityaSripal self-assigned this Aug 7, 2024
@crodriguezvega crodriguezvega added the type: feature New features, sub-features or integrations label Aug 8, 2024
@colin-axner colin-axner moved this to Todo 🏃 in IBC Eureka Aug 12, 2024
@colin-axner
Copy link
Contributor

Delay periods are the only reason I see not to do an automatic migration. Perhaps we could add logic:

  • if delay period is non zero on the connection, skip
  • gov proposal to update alias mapping

In addition, we should consider spreading the migration across several blocks since it doesn't need to all be done in one go.

@colin-axner
Copy link
Contributor

Ah, connection counterparties may vary even when using the same local client id. I believe you will need to alias this as well. Essentially, we would have a map from channelID -> {local_client_id, counterparty_connection_id} and in the counterparty map, connection_id -> {counterparty_client_id, prefix}. I believe things would break if you tried changing the counterparty value in existing connections as the counterparty would make an assertion on the dest_channel_id which would be incorrect

@colin-axner colin-axner self-assigned this Aug 13, 2024
@colin-axner
Copy link
Contributor

Thinking about this some more, we would need to store the counterparty alias as well, this would be utilized in RecvPacket when checking the source information. Given that this would then turn into an alias map which is just the channel/connection layers with condensed information, rather than duplicating this information, lets rely on the channel/connection layers (even if it is two store reads rather than one) in the meantime?

Once we plan to remove the connection/channel structs, then an alias map seems useful, but I believe the best motivation to support existing channels to use eureka would be to access the app version?

I can open a pr with resolving channel ids to client ids to add support for eureka flow with existing channels. Then maybe as a separate, nice-to-have issue, we could attempt to migrate connection structs to this alias map. The map would be:

alias map
channelID -> {local_client_id, counterparty_channel_id}
with the counterparty map adding
counterparty_channel_id -> {counterparty_client_id, prefix}

It's unclear to me if the connection IDs need to be referenced at all?

@colin-axner
Copy link
Contributor

Actually, I'm lost on this safety of this issue.

It's unsafe to send packets for classic channels using eureka, as we need to continue doing classic processing (channel upgrade logic for example)

If we send packets using client ids, this should be seen as a distinct channel (it will generate a different denom)

Until we are ready to migrate from channel/connection layers completely, I'm not sure I understand the correctness and motivation for this issue?

@colin-axner colin-axner removed their assignment Aug 13, 2024
@AdityaSripal
Copy link
Member Author

AdityaSripal commented Aug 13, 2024

I want to keep v1 channel processing as it is. But i would like to be able to send v2 packets on with v1 channel identifiers on day 1.

I don't see why we can't have v2 packets that just use those channel identifiers as identifiers down to the underlying client that can exist side-by-side with the regular v1 channel packet flow (for UNORDERED channels at least).

That way we don't break existing functionality

But we immediately add in the ability to create new versioned packets on the fly
And the ability to create multiatomicity packets as well that preserves fungibility with v1 packets that haven't migrated over

@crodriguezvega crodriguezvega moved this from Todo 🏃 to In Progress 👷 in IBC Eureka Aug 15, 2024
@crodriguezvega crodriguezvega moved this from In Progress 👷 to Todo 🏃 in IBC Eureka Aug 15, 2024
@crodriguezvega crodriguezvega moved this from Todo 🏃 to In Progress 👷 in IBC Eureka Aug 15, 2024
@AdityaSripal
Copy link
Member Author

Fallback method must still be pulled into feature branch from #7174 before it can be closed

@DimitrisJim
Copy link
Contributor

fallback method was added in #7358, closing!

@github-project-automation github-project-automation bot moved this from In progress to Done in IBC-GO Eureka Oct 3, 2024
@github-project-automation github-project-automation bot moved this from In Progress 👷 to Done 🥳 in IBC Eureka Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature New features, sub-features or integrations
Projects
Status: Done
Status: Done 🥳
Development

No branches or pull requests

4 participants