Ucan-core refactor #82
Conversation
|
Wohooo! Nice. Some minor bikeshedding before I deeply look into this: |
|
Oh and - by the way - does |
|
ah yup thats a good point. i like ah right. hmm i mostly use my personal email even with work things lol. I'll switch it over to either bluesky or personal 👌 |
There was a problem hiding this comment.
Thanks so much for working on this, I think it's great we're moving in this direction! Better abstractions!
I added a bunch
Plugin Abstraction
I ended up doing a global state variable for plugins. If you remember from the issue, this wasn't my original plan and I frankly still don't love it. But I was having to thread the plugins through a lot of functions, which mean re-exporting all those functions in the top-level package, and some of those it was difficult, because
staticmethods on classes needed them, etc. I figured it was much more straightforward to do it in this manner. Anyone that wants to load in other dependencies would likely wrap their implementation similarly to our top-level.
I'm not 100% convinced. I know it sucks to thread through an argument for lots of functions.
- For classes: We could parameterize the whole class instead of each parameter. So you'd
export const MkStore = plugins => class Store { .... - For functions: I think there's some cases where you can prevent having to "pass through" the plugins parameter to a bunch of functions. You should only have to pass them through a function, if it's exposed. So e.g. in the
attenuations.tsfile, you could write thedelegationChainsdefinition like this:export const mkDelegationChains = plugins => function * delegationChains(...) { async function* capabilitiesFromDelegation(...) { /* can use plugins here */ ... } ... } - Generally we can try to reduce the API surface. Maybe it'll get better when we remove the
did.tsAPIs that I think won't actually work out in the long run? The fewer APIs we have, the less functions to have to thread through our parameters.
I know I'm kind of "why don't you just?"-ing you, let me know if I'm pushing too hard on this and it's not worthwhile, or in case I don't understand how bad that version would look, maybe we can get on a call and discuss more concrete cases in depth. ✌️
Tooling
The monorepo is giving my VSCode a hard time 😅 What setup are you using?
- Is eslint working for you? For me it fails on the
.eslintrc.js, because it links to./tsconfig.eslint.jsonfile which was moved. - VSCode shows me errors on import statements such as
import ... from "@ucans/core". Apparently it can't resolve@ucans/core. Does that work for you?
We should totally offload those problems to another PR that is just focused on improving the tooling situation though, as long as all of the individual package.json scripts & publishing work fine 👍
PS: There's no line for me to put this comment on, so I'm putting it here: The packages/plugins/src/rsa/index.ts file is empty, we can probably remove it.
|
I'll take another look at the plugins abstraction. I'd also like for it not to be a global var. Reducing the API surface will certainly help. Big fan of eliminating the DID utility fns. I think I can make threading the plugins through work. If it really looks like it's not shaking out, I'll give a shout & we can reassess.
Oops! That's on me. I had the same issue, but readded
This does work for me. It's a little bit fickle. Make sure you run Thanks for the feedback! Glad this roughly feels like the right direction. I'll get this patched up in the next day or two 👍 |
Ah! That was exactly it. That, and running |
packages/core/tests/did.test.ts
Outdated
|
|
||
| beforeAll(loadTestPlugins) | ||
|
|
||
| it("handles old RSA Keys", async () => { |
There was a problem hiding this comment.
I see you've added a couple of tests in the plugins package, and they make most of this file redundant.
But at fission we still need this test for backwards compatibility 😅
That should be kept in the rsa.test.ts 🙏
There was a problem hiding this comment.
Does this cover it alright?
|
Alrighty threaded plugins through functions calls: 61ed38e This ended up being cleaner than I expected. A couple notes on implementation. Please lmk if you have other ideas on any of this 🙏 I ended up breaking the static methods out of You can see the surface area of functions that take the injected plugins in I decided not to have different names for the parameterized fns. such as In the tests, I mimic the high-level |
|
Started on simplifying tooling by moving jest config to the root: 62d07c0 I tried to do the same with |
There was a problem hiding this comment.
Nice!
In the tests, I mimic the high-level
ucanslib intests/lib.tsby injected plugins and re-exporting it. Most of the tests run off of this lib instead of importing fromsrc/.
I think we should just move all tests that use DID stuff from packages/core into packages/ucans (which may be all tests).
I decided not to have different names for the parameterized fns. such as
mkSign => sign. Instead I just call itsignand overwrite it in the exports. I'm not 100% confident in this choice, but I did it so that our exports don't get clogged up with a bunch of pretty low level functions. It also keeps the internal code more readable imo. I'm happy to change this if you have other thoughts.
No the code looks good!
This ended up being cleaner than I expected. A couple notes on implementation. Please lmk if you have other ideas on any of this 🙏
I ended up breaking the static methods out of
Builder&Storeand just making them functions. The types were getting very wonky when I was trying to parameterize the whole class, this made it pretty straightforward.
Hm. I'm not sure what you mean by the types getting wonky if you parameterize the whole class 🤔
I added some suggested changes for what I was thinking :)
packages/core/src/builder.ts
Outdated
| @@ -59,26 +75,16 @@ function isCapabilityLookupCapableState(obj: unknown): obj is CapabilityLookupCa | |||
| */ | |||
| export class Builder<State extends Partial<BuildableState>> { | |||
There was a problem hiding this comment.
Why not
| export class Builder<State extends Partial<BuildableState>> { | |
| export const MkBuilder = (plugins: Plugins) => class Builder<State extends Partial<BuildableState>> { |
?
And then there's no need for a private plugins: Plugins member.
There was a problem hiding this comment.
The Mk- based naming here I'm not too sure nor am I attached to it. It may be simpler to just have it be builder or Builder or similar.
packages/core/src/store.ts
Outdated
| (knownSemantics: DelegationSemantics) => { | ||
| return new Store(plugins, knownSemantics, {}) | ||
| } | ||
|
|
||
| export class Store { |
There was a problem hiding this comment.
Same thing here?
| export class Store { | |
| export const MkStore = (plugins: Plugins) => class Store { |
|
Sorry had a ton to do this week & just finally getting back to this. I'll get some fixes pushed up on monday :)
ok yup this makes sense to me. I was feeling that same tension. (There's also a pseudo-circular dependency in here since
right so the issue I was running into here was that other files in the package use, for instance, the I'll poke it a bit more & see what I can't come up with |
riiiight. Yes. Interfaces is the way to go I think 👍 |
packages/core/src/builder.ts
Outdated
| && util.hasProp(obj, "expiration") && typeof obj.expiration === "number" | ||
| } | ||
|
|
||
| type BuilderConstructor = new <State extends Partial<BuildableState>>(state: State, defaultable: DefaultableState) => BuilderI<State> |
There was a problem hiding this comment.
Just a couple notes on this:
Looks like the function has to be a default export for some reason:
microsoft/TypeScript#30355
& this type annotation needs to be included to suggest to the compiler how to write the declaration file
As well, I had to export some previously internal types in ./types so the compiler was able to write the declaration file.
I was running into this error: microsoft/TypeScript#5711
|
alrighty! moved most of the tests to I briefly tried to do some tsconfig monorepo tricks like config inheritance & a composite config with references to other packages. But the current build setup was preventing me from doing that. Namely, copying the tsconfig into the |
Sounds like a plan. Created an issue for this: #84 Do you know why CI is failing? It says
Is it just because we've never published @ucans/core? It should just pick up that package locally, right? 🤔 |
|
It didn't like my alpha version in one of the core I also added in some workspace commands for building & testing all three packages from the workspace root. Seems to all be passing now 👌 |
Problem
Several problems are addressed by this refactor:
one-webcryptoin react-native for instance), and developers may want to choose a different crypto implementationdid:keyand the library only understooddid:keys that had been hard coded inCloses #77
Solution
Refactor the library into three pieces
@ucans/core: a 1-dependency (I didn't get rid ofuint8arraysyet 😛) library that contains the core logic for working with ucans@ucans/plugins: a set of basic default "plugins" for the ucan library: RSA, NIST P-256, and ED25519@ucans/ucans(still workshopping the name on this one): Loads the default plugins into@ucans/core& re-exportscore+pluginsImplementation notes
Take a look at
core/plugins& each of the plugin files in thepluginspackage to get an idea of the semantics.You'll notice that I handle
did:keydifferently from the other methods, because it's such a common use case.I ended up doing a global state variable for plugins. If you remember from the issue, this wasn't my original plan and I frankly still don't love it. But I was having to thread the plugins through a lot of functions, which mean re-exporting all those functions in the top-level package, and some of those it was difficult, because
staticmethods on classes needed them, etc. I figured it was much more straightforward to do it in this manner. Anyone that wants to load in other dependencies would likely wrap their implementation similarly to our top-level.I left
uint8arraysin core. I'd like to make it 0-dependency, but I didn't want to go through re-creating all the encoding fns just yet. Also the library is pretty lightweight & comes from a trusted source. We can switch it out later if we feel so inclined.I got rid of the
KeyTypetype. These mapped 1:1 withjwtAlgand I figured that we could simplify by just usingjwtAlg.I also got rid of the BaseKey class, and just have each keypair implementing the interfaces directly. It was causing more headaches than good. I also removed the required
publicKeyon theKeypairinterface. This can lead to some confusion with, for instance, P-256 keys. Is it the compressed or decompressed public key? We don't really need it anywhere in the code, so I removed it to avoid that confusion.Tests of crypto functions are in the
pluginslibrary. I left the rest of the tests incore. Most of the tests don't make a ton of sense without being able to use some crypto functions, so I import@ucans/pluginsas a devDependency and load it in for core tests. I don't love this approach, but it was the best I saw for now. Let me know if you have other ideas!I mostly left the configs & build systems untouched. I know a bunch of trial & error has gone into them & my brain glazes over when I try to mess with build systems 😅. However, this did lead to a good bit of repeat code. There's a
tsconfig.json,jest.config.js, andtsconfig.eslint.jsonin each package root and at the repo root. It would be nice to remove that duplication. Similarly, I just copied over thepackage.jsonto each package, with some minor edits. I left the package version where it's at on all of them (0.9.1, though I figure this refactor will bump it to0.10.0). I'm very amenable to any suggestions/recommendations on any of this stuff.I briefly considered splitting each cryptosystem out into its own library (instead of all in one
pluginslibrary), but decided not to proliferate the packages too much at this point. However, to mimic this separation of concerns, I split up the package by crypto system instead of function type (before it wascrypto/,keypairs/, etc; now it isp256/,ed25519/, etc).Thanks for helping plan this PR! Very excited to get it going. I hope the general direction I took makes sense, but happy for any feedback on it/changes requested.
If it's cool with yall, I'd like to publish an alpha version of
corethat we can start messing around with internally :)