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)

[lidel]

The PR's WithMetadata(map[string]ipld.Node) exposes go-ipld-prime in the public API, which:

1. Forces users to import go-ipld-prime just to use metadata
2. Couples boxo's public API stability to a third-party library
3. Makes the API harder to use for simple cases

Good news is that we already have prior art idiomatic patterns for handling this in boxo.

The ipns package already handles this well - the Record struct wraps datamodel.Node internally (line 42) but exposes typed methods (Value(), Sequence(), TTL()) instead of the raw node. This is the pattern to follow.

@phaseloop could you refactor to expose local type? It could be a thin wrapper internally (pseudocode below, feel free to change/improve, just food for thought):

// MetadataValue represents a value that can be stored in IPNS record metadata.
  // Use the constructor functions to create values.
  type MetadataValue struct {
      node ipld.Node
  }

  // StringValue creates a string metadata value.
  func StringValue(s string) MetadataValue {
      return MetadataValue{node: basicnode.NewString(s)}
  }

  // BytesValue creates a bytes metadata value.
  func BytesValue(b []byte) MetadataValue {
      return MetadataValue{node: basicnode.NewBytes(b)}
  }

  // IntValue creates an integer metadata value.
  func IntValue(i int64) MetadataValue {
      return MetadataValue{node: basicnode.NewInt(i)}
  }

  // BoolValue creates a boolean metadata value.
  func BoolValue(b bool) MetadataValue {
      return MetadataValue{node: basicnode.NewBool(b)}
  }

  // WithMetadata sets custom metadata entries for the IPNS record.
  // Keys should be prefixed with "_" per the IPNS spec to avoid
  // collisions with future standard fields.
  func WithMetadata(metadata map[string]MetadataValue) Option {
      return func(o *options) {
          m := make(map[string]ipld.Node, len(metadata))
          for k, v := range metadata {
              m[k] = v.node
          }
          o.metadata = m
      }
  }

Usage becomes:

meta := map[string]ipns.MetadataValue{
      "_myapp/version": ipns.StringValue("1.0"),
      "_myapp/data":    ipns.BytesValue(someBytes),
      "_myapp/count":   ipns.IntValue(42),
  }
  rec, err := ipns.NewRecord(sk, path, seq, eol, ttl, ipns.WithMetadata(meta))

For reading Metadata back, we could add typed accessors on Record:

// Metadata returns a custom metadata value by key.
  // Returns ErrMetadataNotFound if the key doesn't exist.
  func (rec *Record) Metadata(key string) (MetadataValue, error) {
      node, err := rec.node.LookupByString(key)
      if err != nil {
          return MetadataValue{}, fmt.Errorf("%w: %s", ErrMetadataNotFound, key)
      }
      return MetadataValue{node: node}, nil
  }

  // AsString returns the value as a string.
  func (mv MetadataValue) AsString() (string, error) {
      return mv.node.AsString()
  }

  // AsBytes returns the value as bytes.
  func (mv MetadataValue) AsBytes() ([]byte, error) {
      return mv.node.AsBytes()
  }

  // AsInt returns the value as an int64.
  func (mv MetadataValue) AsInt() (int64, error) {
      return mv.node.AsInt()
  }

  // AsBool returns the value as a bool.
  func (mv MetadataValue) AsBool() (bool, error) {
      return mv.node.AsBool()
  }

We probably want to limit the complexity here and avoid implementing complex nested metadata. All you need for complex cases is the ability to store and read []byte.

[lidel]

nit: silently overwriting a genmeric key named Value or TTL or Sequence will lead to hard to debug errors for someone.

let's save that poor soul, and instead of silently overwriting, detect it, and return error with clear error message that "TTL", "Value", "Sequence", "Validity", "ValidityType" are reserved for IPNS Record itself

[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