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

Refactor validated_expected_channel() and check_destination_channel_state() #4056

Open
5 tasks
joaotav opened this issue Jun 21, 2024 · 0 comments
Open
5 tasks
Labels
A: low-priority Admin: low priority / non urgent issue, expect longer wait time for PR reviews

Comments

@joaotav
Copy link
Collaborator

joaotav commented Jun 21, 2024

Summary

When building channel handshake messages, validated_expected_channel() is used to retrieve the specified channel end from the destination and check if it matches the state that is required for the handshake message to be successful.

validated_expected_channel() and the sub-method check_destination_channel_state() could use some improvement given that:

  • validated_expected_channel() currently returns a ChannelEnd object that is never used, it's not clear why this is needed.

  • Both the OpenAck and OpenConfirm messages expect the receiving channel end's highest state to be TRYOPEN. It would be helpful to clarify in the comments why this is needed. Is it due to crossing hellos?

// The highest expected state, depends on the message type:
let highest_state = match msg_type {
    ChannelMsgType::OpenAck => State::TryOpen,
    ChannelMsgType::OpenConfirm => State::TryOpen,
    ChannelMsgType::CloseConfirm => State::Open(UpgradeState::NotUpgrading),
    _ => State::Uninitialized,
};
  • check_destination_channel_state() could also use refactoring or could be merged into validated_expected_channel()

Proposal

Refactor validated_expected_channel() and check_destination_channel_state() and add comments to enhance clarity. Since this involves the tx CLIs, it can potentially be done together with the work to support multihop connections on those same CLIs (#3940).


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate milestone (priority) applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@joaotav joaotav added the A: low-priority Admin: low priority / non urgent issue, expect longer wait time for PR reviews label Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: low-priority Admin: low priority / non urgent issue, expect longer wait time for PR reviews
Projects
Status: 🩹 Triage
Development

No branches or pull requests

1 participant