-
Notifications
You must be signed in to change notification settings - Fork 37
feat: Promote NsRecord into a core object #399
Conversation
bfd93f1
to
614bae4
Compare
b36c38b
to
1ea092e
Compare
There was a problem hiding this 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:
- Refactor
LinkRecord
to hold aUcan
internally (not aJwt
), or.... - Add yet-another-wrapper that takes a
LinkRecord
and internally also holds a correspondingUcan
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.
d5a5775
to
f61c309
Compare
Reconfigured to wrap a Ucan, not dissimilar from NsRecord; updating accordingly
|
There was a problem hiding this 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 🙏
…::data::LinkRecord with its functionality, and removing the noosphere_ns dependency on validating records. Fixes #395
updated: removed the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
... by extending noosphere_core::data::LinkRecord with its functionality, and removing the noosphere_ns dependency on validating records. Fixes #395