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

When a channel reopens the ordering and metadata must not change #5523

Closed
Tracked by #3996
crodriguezvega opened this issue Jan 5, 2024 · 6 comments · Fixed by #5562
Closed
Tracked by #3996

When a channel reopens the ordering and metadata must not change #5523

crodriguezvega opened this issue Jan 5, 2024 · 6 comments · Fixed by #5562

Comments

@crodriguezvega
Copy link
Contributor

crodriguezvega commented Jan 5, 2024

Implement this pseudocode from the spec in OnChanOpenInit of the controller:

// only open the channel if:
// - there is no active channel already set (with status OPEN)
// OR
// - there is already an active channel (with status CLOSED) AND
// the metadata matches exactly the existing metadata in the 
// version string of the active channel AND the ordering of the 
// new channel matches the ordering of the active channel.
activeChannelId, activeChannelFound = GetActiveChannelID(portId, connectionId)
if activeChannelFound {
  activeChannel = provableStore.get(channelPath(portId, activeChannelId))
  abortTransactionUnless(activeChannel.state === CLOSED)
  previousOrder = activeChannel.order
  abortTransactionUnless(previousOrder === order)
  previousMetadata = UnmarshalJSON(activeChannel.version)
  abortTransactionUnless(previousMetadata === metadata)
}
@ThanhNhann
Copy link
Contributor

ThanhNhann commented Jan 7, 2024

Can I handle this issue? And I see the link you add with OnChanOpenInit of the controller maybe wrong

@crodriguezvega crodriguezvega moved this to Todo in ibc-go Jan 7, 2024
@crodriguezvega
Copy link
Contributor Author

Can I handle this issue? And I see the link you add with OnChanOpenInit of the controller maybe wrong

@ThanhNhann Thank you for the offer. The issues linked to #3996 are a bit time sensitive, since we want to include the whole feature in the release of v8.1 by the end of the month, so I think it will be better if we manage these internally. Thank you again!

@srdtrk
Copy link
Member

srdtrk commented Jan 8, 2024

Why do we want to enforce this? I'd like to allow ordering (and even encoding) to change.

@srdtrk srdtrk added the needs discussion Issues that need discussion before they can be worked on label Jan 8, 2024
@crodriguezvega
Copy link
Contributor Author

We agreed that for v8.1 we will implement this and we can find ways to relax the restriction afterwards.

@crodriguezvega crodriguezvega removed the needs discussion Issues that need discussion before they can be worked on label Jan 8, 2024
@chatton chatton self-assigned this Jan 10, 2024
@chatton chatton moved this from Todo to In review in ibc-go Jan 10, 2024
@AdityaSripal
Copy link
Member

Given this is a highly requested feature, seems weird to me to disable and then find a way to reenable later when a solution already exists. i'd prefer to change the spec in this case

@colin-axner
Copy link
Contributor

#5562 (comment)

closed by #5562

@github-project-automation github-project-automation bot moved this from In review to Done in ibc-go Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants