Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: implement new ipns record&answer properties #23
feat: implement new ipns record&answer properties #23
Changes from 3 commits
2138c96
f0dfc60
1d73d77
9f5078a
e836abf
8bf9c9f
d2cbc3c
dbd8863
a93b5e4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use the record's TTL value also for the TLRU cache (instead of the static 2 minute value we set).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@multiformats/dns
caches all query answers up to the answer TTL, and@helia/ipns
stores resolved IPNS records in the local datastore, so is it necessary to have another TLRU cache at this level?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not. What caching logic is there for IPNS? Does it always return from cache if the record is still valid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can test removing this but even though helia/ipns was supposed to be caching records previously, it wasnt, and multiple requests were seen in sw gateway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may have been because up until recently we were reinstantiating verified fetch for every request to the SW
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah thats a good point...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's here: https://github.com/ipfs/helia/blob/main/packages/ipns/src/index.ts#L526-L584
It searches the local datastore for a cached record but also searches the routing, so we might just need to make it only go to the routing if the local store doesn't have a valid record.
The problem there is you won't see an updated IPNS record until your local copy expires, or the user requests an uncached record.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm going to use the existing cache for now, but set the TTL for the tlru cache to
ttl ?? 60 * 1000 * 2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated caching logic for
@helia/ipns
- ipfs/helia#473There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: ipfs/specs#467
Check warning on line 23 in packages/verified-fetch/src/utils/response-headers.ts
Codecov / codecov/patch
packages/verified-fetch/src/utils/response-headers.ts#L18-L23
Check warning on line 429 in packages/verified-fetch/src/verified-fetch.ts
Codecov / codecov/patch
packages/verified-fetch/src/verified-fetch.ts#L426-L429
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fails
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the error you are seeing? It passes for me..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you know what, i read this on my phone. looking from the PC, this is probably fine. will test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idk what happened but reverting it back to the original
.to.eventually.be.rejected.with.property('message', 'Could not parse PeerId in ipns url "mydomain.com", Non-base64 character')
works.