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

fix!: rename "transient" connections to "limited" #2645

Merged
merged 4 commits into from
Aug 14, 2024

Conversation

achingbrain
Copy link
Member

@achingbrain achingbrain commented Jul 31, 2024

To better align with go-libp2p@0.34.0 rename "transient" connections to "limited".

BREAKING CHANGE: There are three breaking API changes:

  • The .transient boolean property has been replaced with a .limits object that may be undefined or may have .bytes and/or .seconds properties - the value of these properties decreases as the limits are used up
  • The runOnTransientConnection property of libp2p.handle and libp2p.dialProtocol has been renamed to runOnLimitedConnection
  • The notifyOnTransient property of libp2p.register has been renamed notifyOnLimitedConnection

Refs: #2622

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if necessary (this includes comments as well)
  • I have added tests that prove my fix is effective or that my feature works

@achingbrain achingbrain added the version-2.0 PRs that will be released in libp2p v2 label Jul 31, 2024
@achingbrain achingbrain requested a review from a team as a code owner July 31, 2024 16:58
@achingbrain achingbrain mentioned this pull request Jul 31, 2024
27 tasks
@achingbrain
Copy link
Member Author

achingbrain commented Jul 31, 2024

I've backed away from removing runLimitedConnection and notifyOnLimitedConnection because there are cases when you cannot make the decision ahead of time, such as:

const stream = await libp2p.dialProcotol(peerId, '/my/protocol/1.0.0')

In the above since you're dialing the protocol on the remote peer and not a multiaddr, you don't know ahead of time if you are connected or if the connection is relayed/direct/limited so the runLimitedConnection option provides an escape hatch.

Happy to discuss further if anyone has a better idea for the API.

@achingbrain achingbrain marked this pull request as draft July 31, 2024 17:17
To better align with [go-libp2p@0.34.0](https://github.com/libp2p/go-libp2p/releases/tag/v0.34.0)
rename "transient" connections to "limited".

BREAKING CHANGE: There are three breaking API changes:
  * Connections have an optional `.limits` property
  * The `runOnTransientConnection` property of `libp2p.handle` and `libp2p.dialProtocol` has been renamed to `runOnLimitedConnection`
  * The `notifyOnTransient` property of `libp2p.register` has been renamed `notifyOnLimitedConnection`

Refs: #2622
@achingbrain achingbrain force-pushed the fix/rename-transient-to-limited branch from 692bdb4 to f24a06e Compare July 31, 2024 17:18
@achingbrain achingbrain marked this pull request as ready for review August 1, 2024 08:21
Copy link
Member

@maschad maschad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work.

  1. A minor comment adjustment here

  2. we don't really have any docs around this but I suppose we still should have a migration guide correct?

@SgtPooki
Copy link
Member

I've backed away from removing runLimitedConnection and notifyOnLimitedConnection because there are cases when you cannot make the decision ahead of time, such as:

const stream = await libp2p.dialProcotol(peerId, '/my/protocol/1.0.0')

In the above since you're dialing the protocol on the remote peer and not a multiaddr, you don't know ahead of time if you are connected or if the connection is relayed/direct/limited so the runLimitedConnection option provides an escape hatch.

Happy to discuss further if anyone has a better idea for the API.

I think your rationale is sound. Though I'm not sure where the "removing" work was happening or what that consisted of. I'm guessing you were thinking of removing

runOnTransientConnection?: boolean
?

Do we have a best practice or prior art for whether we default API decisions to a "try, analyze, retry" vs "config for goal success" ?

I know we don't like to "config all the things" but it seems like js-libp2p likes to go the "config for goal success" route: i.e. when we make a function call, we want devs to pass whatever they need to ensure they can achieve their ideal outcome on first try. so.. for dialProtocol, having a runOnLimitedConnection option available that devs can configure would be good.

@achingbrain
Copy link
Member Author

Though I'm not sure where the "removing" work was happening or what that consisted of.

See the linked issue mentioned in the op.

Do we have a best practice or prior art for whether we default API decisions to a "try, analyze, retry" vs "config for goal success" ?

Best practice is generally to avoid network operations wherever possible as I/O is the death of performance. In this case to make two attempts at opening a stream (and potentially a connection) could be very expensive so should be avoided.

@achingbrain achingbrain merged commit 58ec62c into release-v2.0 Aug 14, 2024
24 checks passed
@achingbrain achingbrain deleted the fix/rename-transient-to-limited branch August 14, 2024 12:44
achingbrain added a commit that referenced this pull request Sep 6, 2024
To better align with [go-libp2p@0.34.0](https://github.com/libp2p/go-libp2p/releases/tag/v0.34.0)
rename "transient" connections to "limited".

BREAKING CHANGE: There are three breaking API changes:
  * Connections have an optional `.limits` property
  * The `runOnTransientConnection` property of `libp2p.handle` and `libp2p.dialProtocol` has been renamed to `runOnLimitedConnection`
  * The `notifyOnTransient` property of `libp2p.register` has been renamed `notifyOnLimitedConnection`

Refs: #2622
achingbrain added a commit that referenced this pull request Sep 6, 2024
To better align with [go-libp2p@0.34.0](https://github.com/libp2p/go-libp2p/releases/tag/v0.34.0)
rename "transient" connections to "limited".

BREAKING CHANGE: There are three breaking API changes:
  * Connections have an optional `.limits` property
  * The `runOnTransientConnection` property of `libp2p.handle` and `libp2p.dialProtocol` has been renamed to `runOnLimitedConnection`
  * The `notifyOnTransient` property of `libp2p.register` has been renamed `notifyOnLimitedConnection`

Refs: #2622
achingbrain added a commit that referenced this pull request Sep 6, 2024
To better align with [go-libp2p@0.34.0](https://github.com/libp2p/go-libp2p/releases/tag/v0.34.0)
rename "transient" connections to "limited".

BREAKING CHANGE: There are three breaking API changes:
  * Connections have an optional `.limits` property
  * The `runOnTransientConnection` property of `libp2p.handle` and `libp2p.dialProtocol` has been renamed to `runOnLimitedConnection`
  * The `notifyOnTransient` property of `libp2p.register` has been renamed `notifyOnLimitedConnection`

Refs: #2622
@achingbrain achingbrain mentioned this pull request Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
version-2.0 PRs that will be released in libp2p v2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants