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

Proposal: remove explicit link functionality #3

Open
wants to merge 2 commits into
base: first-draft
Choose a base branch
from

Conversation

expede
Copy link
Member

@expede expede commented Aug 13, 2023

📖 Preview

A promised, this a proposed change to #1 that entirely removes the ability to add a CID to an embedded DAG.

I have only changed the schemata and example, but not the prose or FAQ. I'll fix those up once I confirm that this is aligned with y'all.

Screenshot 2023-08-13 at 17 47 54

This PR doesn't bother with the hash, since we if the condiguration is the same as the currounding context can always calculate it directly from the data, and presumably we want to do that check anyway. If you want to be able to re-encode the outer DAG, then we can't provide a concrete CID anyways.

Evolvabilty

we could add later if we find that it's painful without it — much harder to remove it later than add it in later.

The ability to nest arbitrarily configured CIDs isn't in the main use case that I want this for, so please let me know if it's a bigger deal.

I ended up trying a version that's just "." as a key. The downside is that this has a might higher chance for a collision with someone else's (userland) schema. A nice thing about "/" is that conflicting on / is already a case that exists in DAG-JSON. The more fields or nesting that we add, the fewer cases we potentially overlap.

The PR as it stands leaves room for us to add the explicit link back over time without changing the format:

Screenshot 2023-08-13 at 17 43 37

Otherwise, we could keep the nesting to maintain evolvability, which is closer to #1.

// Current PR
{
  "foo": 123,
  "bar": {".": {"some-inner": ["dag", "data"]}}, // Inner DAG
  "baz": "quux"
}

// Back to nesting
{
  "foo": 123,
  "bar": {"/": {".": {"some-inner": ["dag", "data"]}}} // Inner DAG
  "baz": "quux"
}

Which both leave open evolving to:

// This PR
{
  "foo": 123,
  "bar": {
    // Flat, with CID
    "/": bafyCid,
    ".": {"some-inner": ["dag", "data"]}
  },
  "baz": "quux"
}

// With nesting & CID
{
  "foo": 123,
  "bar": {
    "/": {
      "&": bafyCid,
      ".": {"some-inner": ["dag", "data"]}
    }
  },
  "baz": "quux"
}

These all of course "look" less like other codecs, but they can be easily transformed into DAG-CBOR, etc, which is the point.

I can live with either version, though. Lemme know.

@expede expede marked this pull request as ready for review August 17, 2023 02:43
Copy link

@Gozala Gozala left a comment

Choose a reason for hiding this comment

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

I'd personally prefer nested version so we don't have to reserve . along with /. That said I don't feel very strongly about it, so this version looks good to me as well.

@expede
Copy link
Member Author

expede commented Oct 3, 2023

@Gozala I also prefer the nested version! I thought you were previously arguing for the opposite. Will update this PR

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