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(ipns): reading records with raw []byte Value #830

Merged
merged 3 commits into from
Feb 4, 2025

Conversation

lidel
Copy link
Member

@lidel lidel commented Feb 3, 2025

This PR aims to improve / maximize interop with legacy/alternative IPNS clients by restoring support for []byte CID in Value field and gracefully handling empty Value in scenarios where IPNS is used for its Data field, and not IPFS paths.

  • record.Value() will convert a binary CID to a valid path.Path.
  • Empty Value() will produce NoopPath (/ipfs/bafkqaaa) to avoid breaking existing code that expects a valid record to always produce a valid content path.
  • tests to ensure we test both + error if Value() can't produce a meaningful path

Thank you @ianopolous and @aschmahmann for catching this.

If / once this lands, I'll update phrasing in https://specs.ipfs.tech/ipns/ipns-record/ to make it clear binary CID and empty Value are both allowed.

Aiming to include this in Kubo 0.34.

- []byte CID works again
- empty value is allowed and produces NoopPath
@lidel lidel mentioned this pull request Feb 3, 2025
24 tasks
Copy link

codecov bot commented Feb 3, 2025

Codecov Report

Attention: Patch coverage is 84.21053% with 3 lines in your changes missing coverage. Please review.

Project coverage is 60.51%. Comparing base (1ff84ce) to head (c520ac2).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
ipns/record.go 84.21% 2 Missing and 1 partial ⚠️

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #830      +/-   ##
==========================================
+ Coverage   60.42%   60.51%   +0.08%     
==========================================
  Files         244      244              
  Lines       31120    31136      +16     
==========================================
+ Hits        18804    18841      +37     
+ Misses      10633    10615      -18     
+ Partials     1683     1680       -3     
Files with missing lines Coverage Δ
ipns/record.go 69.81% <84.21%> (+1.59%) ⬆️

... and 12 files with indirect coverage changes

@lidel lidel marked this pull request as ready for review February 3, 2025 22:20
@lidel lidel requested a review from a team as a code owner February 3, 2025 22:20
ipns/record.go Outdated Show resolved Hide resolved
ipns/record.go Show resolved Hide resolved
ipns/record.go Show resolved Hide resolved
@gammazero gammazero merged commit 8d689c8 into main Feb 4, 2025
15 checks passed
@gammazero gammazero deleted the fix/ipns-allow-binary-values branch February 4, 2025 16:54
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.

2 participants