feat(ipns): Support adding custom metadata to IPNS record

[phaseloop]

Allow adding custom metadata to IPNS record - stored as additional keys in DAG-CBOR data.

IPNS specification allows to store arbitrary data in the record but this was not supported by NewRecord() method.

cc: @lidel

[lidel]

Thank you for opening this @phaseloop, having ability to set/get metadata CBOR feels sensible, but this PR needs bit more work around two things:

1. We should not use third-party type from legacy IPLD library in public API here
2. You implemented way to set, but its missing method to read custom metadata from unmarshaled records

I suggested one way of addressing both below, but other ideas welcome.
In general, we want to move away from IPLD language and focus on deterministic subset of CBOR (dag-cbor).

Note that DAG-CBOR itself becomes problematic if you try to store something more than basic types:

• https://hyphacoop.github.io/dasl-testing/?group=tests-by-file&tag=dag-cbor

This is why my suggestion is to limit the API for storing/reading metadata to raw []byte with entire dag-cbor + limited ability to set specific key as either String, []byte, boolean and maybe Integer (look at linked test results and pick minimal set of CBOR types that works everywhere)

[phaseloop]

Thanks a lot! I'll work on it. Off topic question: why DAG-CBOR was chosen instead of protobuf if implementations are so fragmented and buggy outside basic types? Is the storage saving so big compared to protobuf that it was worth the hassle? CBOR/DAG-CBOR seems to be pretty obscure.

[lidel]

Thanks! I'd prefer to avoid mischaracterizing work done by others, but my understanding is that the signed CBOR map was introduced years ago hastily to fix a security issue in V1, it was never a well thought out decision for metadata. But since we have this signed CBOR map, we can put other values there now.

I agree, CBOR creates unnecessary complexity, but it is too late to change V2 without breaking existing clients. Note that to this day, Kubo is producing V1+V2 records to keep backward compatibility.

This PR is probably not the place to discuss it, so let's avoid off-topic, but if you are interested in cleaning up this, feel free to open discussion in https://github.com/ipfs/specs/pulls (in theory, we could introduce V3 of IPNS records that improves the wire format, but upgrade path for ecosystem is a tricky part, see some prior art in https://specs.ipfs.tech/ipips/ipip-0428/).

[phaseloop]

Thanks! I was just curious, so far I have no intention to rework IPNS (if it works, it works) - but I'm working on using IPNS as distributed DNS system:

ipfs/specs#528

[phaseloop]

@lidel I reworked the PR according to your comments

[phaseloop]

@gammazero PR was modified according to comments, AFAIK "need/author-input" label can be removed now :)

[lidel]

@phaseloop thanks, i had no bandwidth to do proper review in Kubo (hopefully after 0.40 ships), but small asks inline + you may want to rebase this on top of latest main

[lidel]

@phaseloop nit: the existing ipns package uses sentinel var errors everywhere (ErrExpiredRecord, ErrInvalidRecord, etc. in errors.go). This PR introduces ErrMetadataNotFound and ErrMetadataConflict as struct types, which means:

• errors.Is(err, ErrMetadataNotFound{}) won't work as callers might expect (they'd need errors.As)
• The empty struct bodies (type ErrMetadataNotFound struct{}) are unnecessary boilerplate

mind refactoring to sentinel var errors matching the existing pattern:
``go
var ErrMetadataNotFound = errors.New("metadata key not found in record")
var ErrMetadataConflict = errors.New("metadata key uses reserved name")

[lidel]

nit: should not modify TestCBORDataSerialization which tests the default base behavior

extra metadata should be tested in own, separate unit test

[lidel]

nit: this slice is rebuilt on every call to createNode

This is a package-level constant set. Could be a var at package level or sth :)