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: align implicit default ttl with specs #749

Merged
merged 1 commit into from
Mar 4, 2025
Merged

Conversation

lidel
Copy link
Member

@lidel lidel commented Feb 26, 2025

Title

chore(ipns): align implicit default ttl with specs

Description

Notes & open questions

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

Sorry, something went wrong.

lowering TTL to 5m due to ipfs/specs#492
lifetime is validity of signature, and that should be fine up to 48h,
to match DHT expiration window
@lidel lidel force-pushed the fix-ipns-defaults branch from 6c36281 to 693e3b1 Compare February 26, 2025 17:59
@lidel lidel marked this pull request as ready for review February 26, 2025 18:08
@lidel lidel requested a review from a team as a code owner February 26, 2025 18:08
@achingbrain achingbrain changed the title chore(ipns): align implicit default ttl with specs fix: align implicit default ttl with specs Feb 27, 2025
@@ -327,7 +327,7 @@ export interface PublishOptions extends AbortOptions, ProgressOptions<PublishPro
v1Compatible?: boolean

/**
* The TTL of the record in ms (default: 1 hour)
* The TTL of the record in ms (default: 5 minutes)
Copy link
Member

@achingbrain achingbrain Feb 27, 2025

Choose a reason for hiding this comment

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

Maybe I'm missing something but the suggested default is one hour?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Aha! Like it says in the issue description..

@achingbrain achingbrain merged commit 375796a into main Mar 4, 2025
24 checks passed
@achingbrain achingbrain deleted the fix-ipns-defaults branch March 4, 2025 17:25
@achingbrain achingbrain mentioned this pull request Mar 4, 2025
2color added a commit that referenced this pull request Mar 13, 2025
* origin/main:
  chore: use peer id parsing function from libp2p (#762)
  feat: add republish signed ipns records (#745)
  fix: use bytestream methods to add byte streams (#758)
  chore: bump codecov/codecov-action from 5.3.1 to 5.4.0 (#752)
  feat: allow modifying trustless-gateway fetch (#751)
  fix: align implicit default ttl with specs (#749)
  docs: add spell checker to ci (#743)
  chore: Update FUNDING.json for Optimism RPF (#746)
lidel added a commit to ipfs/js-ipns that referenced this pull request Mar 18, 2025
Same rationale as
ipfs/helia#749
lidel added a commit to ipfs/js-ipns that referenced this pull request Mar 18, 2025
Same rationale as
ipfs/helia#749
achingbrain pushed a commit to ipfs/js-ipns that referenced this pull request Mar 18, 2025
This PR is changing implicit default TTL of IPNS Record produced by this library from 1h to 5m.

Same rationale as ipfs/kubo#10718 and ipfs/helia#749. This library has its own default TTL, which is separate from one we already updated ipfs/helia#749. Making this change so we have the same default everywhere.

Historical notes
- Before April 2024 the IPNS Record TTL was not set (0) in this library
- Since #308 the `1h` default produced by this library started causing stale cache issues that end users often report as "ipns is slow (to update/propagate)".
- We've changed default recommendation to 5m in ipfs/specs#492
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants