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

Make connection lookup calls consistent in 04-channel #3840

Closed
3 tasks
damiannolan opened this issue Jun 14, 2023 · 5 comments
Closed
3 tasks

Make connection lookup calls consistent in 04-channel #3840

damiannolan opened this issue Jun 14, 2023 · 5 comments
Assignees
Labels
04-channel type: code hygiene Clean up code but without changing functionality or interfaces

Comments

@damiannolan
Copy link
Member

Summary

The 04-channel submodule has inconsistent usage of k.GetConnection() and k.connectionKeeper.GetConnection() in order to lookup a connectionEnd.

See thread

Make usage consistent across handshake and upgrade handlers.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@damiannolan damiannolan added type: code hygiene Clean up code but without changing functionality or interfaces 04-channel labels Jun 14, 2023
@damiannolan damiannolan added this to the 04-channel upgrades alpha milestone Jun 14, 2023
@colin-axner colin-axner added the needs discussion Issues that need discussion before they can be worked on label Jun 19, 2023
@DimitrisJim
Copy link
Contributor

Related but non-blocking: #3466

@crodriguezvega
Copy link
Contributor

I think my preference here would be the following:

If I am not mistaken, the GetConnection function was added so that channel keeper implementation satisfies the ChannelKeeper interface declared in ics27 and we can retrieve the connection via the channel keeper in ics27. Then maybe we could by-convention restrict its usage to that use? Another thing is that if we use GetConnection in 04-channel then the error of not finding the connection is wrapped one more time (maybe unnecessarily)? If connectionKeeper.GetConnection doesn't find the connection, then an error is returned and in channel keeper's GetConnection that error is wrapped.

@DimitrisJim
Copy link
Contributor

Yeah, I see we wrap in the most recent usages of GetConnection (i.e in upgrade.go). I guess it isn't all that necessary really (and kinda beats the point of having GetConnection handle the error uniformly for us).

Personally, I don't mind using channelKeeper.GetConnection since it's there already.

@crodriguezvega
Copy link
Contributor

We will use k.connectionKeeper.GetConnection.

@crodriguezvega
Copy link
Contributor

Closed by #4146.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
04-channel type: code hygiene Clean up code but without changing functionality or interfaces
Projects
Archived in project
Development

No branches or pull requests

4 participants