-
Notifications
You must be signed in to change notification settings - Fork 16
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
kendall/main into main #97
Conversation
+1 me too but i will be slow |
@nitro-neal @kirahsapong be forewarned, I'm merging this PR in but all it consist of is taking rs on kendall/binded-directory-structure [$] via 🦀 1.77.2
➜ ls binded
kt swift ts
rs on kendall/binded-directory-structure [$] via 🦀 1.77.2
➜ ls bindings
uniffi wasm Also, @nitro-neal @kirahsapong I have added you two to the CODEOWNERS. I'll try to stop merging changes in while this PR is under review. |
binded/ts/pkg/web5_wasm_bg.wasm
Outdated
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.
By the way our wasm file is already 1.44 mb (and we just getting started) :)
I have a gut feeling we will ditch js bindings and keep our oldschool web5-js in this regard
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.
I noticed that too, but not giving up yet!
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.
Yeah like @nitro-neal said I would totally ditch it. The reality is that there is so much bespoke code in web5-js to support even the big and diverse JS runtimes such as server, browser, react native, electron that it's just not realistic at any future juncture to have Rust generate it all as WASM. In the future if we only have to support JS and Rust we'll be so far ahead of where we are today that at this point trying to hit the unrealistic goal of using WASM just doesn't seem like a great use of time.
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.
Well said 👍
let verification_method = bearer_did.document.get_verification_method(key_selector)?; | ||
let kid = verification_method.id; | ||
let alg = match verification_method.public_key_jwk.crv.as_str() { | ||
"secp256k1" => "ES256K".to_string(), |
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.
n00b rust question, but is the to_string needed?
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 is a str which is an array of UTF-8 bytes"
and "this is a String type which is managed on the heap".to_string()
basically it's a distinction between str
and String
types -- the former is a static memory (not sure if on the stack or in the static location in memory, but either way) and the latter is on the heap
there's a lot to even this question, just rust thangs
here's a good SO answer on this https://stackoverflow.com/a/24159933
use super::*; | ||
use std::sync::Arc; | ||
|
||
#[test] |
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.
is the end state to have tests in a different _test.rs file?
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.
maybe, we can come to that if it we want I have no strong opinions, nor do I know what's idiomatic
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.
Awesome job! Lots of good work
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! i'm still going through and learning from this pr and will be long after its merged.
let spruce_jwk = ssi_vmm.get_jwk()?; | ||
let alg = match spruce_jwk.algorithm { | ||
Some(Algorithm::ES256K) => "ES256K", | ||
Some(Algorithm::EdDSA) => "EdDSA", |
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.
should switch out EdDSA to Ed25519 here and elsewhere
https://www.ietf.org/archive/id/draft-ietf-jose-fully-specified-algorithms-02.html
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.
Great callout. I did this but it'll be better to dedicate the time to do this right, so created a ticket and will follow up with it #128
let kid = verification_method.id; | ||
let alg = match verification_method.public_key_jwk.crv.as_str() { | ||
"secp256k1" => "ES256K".to_string(), | ||
"Ed25519" => "EdDSA".to_string(), |
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.
"Ed25519" => "EdDSA".to_string(), | |
"Ed25519" => "Ed25519".to_string(), |
expiration: Option<i64>, | ||
not_before: Option<i64>, | ||
issued_at: Option<i64>, | ||
jti: Option<String>, |
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.
prob fine to exclude for now but we will need x5c
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.
yup! will come to when needed
This PR is a culmination of a few weeks of work. Ain't nobody gon' review this. If you want to track/review iteration, I suggest reviewing the Closed PRs because I've been intentional about breaking the changes up in smaller logical chunks.
Major changes include:
Closes #43
Closes #41
Closes #32
Closes #31
Closes #30
Closes #29
Closes #27
Closes #121
Closes #120
Opened many more tickets