Skip to content
This repository has been archived by the owner on Sep 21, 2024. It is now read-only.

feat: Promote NsRecord into a core object #399

Merged
merged 1 commit into from
May 23, 2023
Merged

feat: Promote NsRecord into a core object #399

merged 1 commit into from
May 23, 2023

Conversation

jsantell
Copy link
Contributor

... by extending noosphere_core::data::LinkRecord with its functionality, and removing the noosphere_ns dependency on validating records. Fixes #395

@jsantell jsantell force-pushed the issue-395 branch 2 times, most recently from bfd93f1 to 614bae4 Compare May 18, 2023 20:39
@jsantell jsantell requested a review from cdata May 18, 2023 20:56
@jsantell jsantell force-pushed the issue-395 branch 2 times, most recently from b36c38b to 1ea092e Compare May 22, 2023 20:44
Copy link
Collaborator

@cdata cdata left a comment

Choose a reason for hiding this comment

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

This mostly looks good.

One thing that stood out to me is that all of LinkRecord's implementation is static in order to work around the fact that what it really wants to be operating on is a Ucan (but what it has available is an encoded Jwt).

It seems like there are two possible revisions to this change that are worth choosing from:

  1. Refactor LinkRecord to hold a Ucan internally (not a Jwt), or....
  2. Add yet-another-wrapper that takes a LinkRecord and internally also holds a corresponding Ucan

After applying one of these approaches, the static methods can be re-imagined as instance methods without extra cost required to parse a Ucan for every call.

rust/noosphere-core/src/data/address.rs Outdated Show resolved Hide resolved
rust/noosphere-core/src/data/address.rs Outdated Show resolved Hide resolved
rust/noosphere-core/src/data/address.rs Outdated Show resolved Hide resolved
rust/noosphere-core/src/data/address.rs Outdated Show resolved Hide resolved
rust/noosphere-core/src/data/address.rs Outdated Show resolved Hide resolved
@jsantell jsantell force-pushed the issue-395 branch 2 times, most recently from d5a5775 to f61c309 Compare May 23, 2023 18:55
@jsantell jsantell requested a review from cdata May 23, 2023 19:11
@jsantell
Copy link
Contributor Author

Reconfigured to wrap a Ucan, not dissimilar from NsRecord; updating accordingly

  • Promote noosphere_core::authority::generate_capability(identity: &str, action: SphereAction)
  • Remove optional DidParser from LinkRecord::validate method
  • Updated some traits accordingly, similar to what NsRecord needed as a Ucan wrapper (rather than a Jwt wrapper)
  • Updated method names (with no static methods, dereference was redundant with link, renamed to get_link())

Copy link
Collaborator

@cdata cdata left a comment

Choose a reason for hiding this comment

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

Just a little more cleanup requested 🙏

rust/noosphere-core/src/data/address.rs Outdated Show resolved Hide resolved
rust/noosphere-core/src/data/address.rs Outdated Show resolved Hide resolved
rust/noosphere-core/src/data/address.rs Show resolved Hide resolved
…::data::LinkRecord with its functionality, and removing the noosphere_ns dependency on validating records. Fixes #395
@jsantell
Copy link
Contributor Author

updated: removed the encode() and as_bytes() methods

Copy link
Collaborator

@cdata cdata left a comment

Choose a reason for hiding this comment

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

Nice work!

@cdata cdata merged commit 9ee4798 into main May 23, 2023
@cdata cdata deleted the issue-395 branch May 23, 2023 22:46
@github-actions github-actions bot mentioned this pull request May 23, 2023
@github-actions github-actions bot mentioned this pull request Aug 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate NsRecord to a core type
2 participants