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

Amend GetTimestampAtHeight to accept ConnectionI interface #4021

Closed
3 tasks
DimitrisJim opened this issue Jul 6, 2023 · 4 comments
Closed
3 tasks

Amend GetTimestampAtHeight to accept ConnectionI interface #4021

DimitrisJim opened this issue Jul 6, 2023 · 4 comments
Labels
03-connection change: api breaking Issues or PRs that break Go API (need to be release in a new major version) type: code hygiene Clean up code but without changing functionality or interfaces
Milestone

Comments

@DimitrisJim
Copy link
Contributor

Summary

The connectionKeeper's GetTimestampAtHeight method currently expects a ConnectionEnd type. This is in contrast to the rest of the methods which are defined based on the ConnectionI interface defined in exported.

This results in a requirement to mix calls (see #3840) to access connection keeper methods since channelKeeper.GetConnection returns the interface when GetTimestampAtHeight requires the type (which is accessed via connectionKeeper.GetConnection).

Proposal

Change GetTimestampAtHeight's sig to accept the interface instead of the type.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@DimitrisJim DimitrisJim added type: code hygiene Clean up code but without changing functionality or interfaces 03-connection change: api breaking Issues or PRs that break Go API (need to be release in a new major version) labels Jul 6, 2023
@DimitrisJim
Copy link
Contributor Author

note: unsure if there's history behind this? Also, pretty sure this is API-breaking, though I wouldn't expect anyone to actually be using it.

@crodriguezvega
Copy link
Contributor

Good catch, @DimitrisJim! it could be that maybe this is even not API breaking? The ConnectionEnd fully implements the ConnectionI interface, so maybe it's possible to just replace the argument type without any harm.

@DimitrisJim
Copy link
Contributor Author

due to it being a public symbol it will necessarily be API-breaking I'd assume. People do wild shit with code. I do agree that yeah, call sites won't be affected due to the ConnectionEnd implementing that interface, on our end this just changes signatures.

@crodriguezvega crodriguezvega added this to the v9.0.0 milestone Nov 14, 2023
@DimitrisJim
Copy link
Contributor Author

Closing, this mixed usage should now not be an issue considering we removed that interface #5770

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
03-connection change: api breaking Issues or PRs that break Go API (need to be release in a new major version) type: code hygiene Clean up code but without changing functionality or interfaces
Projects
None yet
Development

No branches or pull requests

2 participants