-
Notifications
You must be signed in to change notification settings - Fork 10
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
Wasm implementation that works e2e with signify-ts #155
base: main
Are you sure you want to change the base?
Conversation
Removed profiling step that works with vanilla rust but wasm uses wasm-opt instead. Added license to remove a warning (copied from directory up). Added some getter macro bindings to dater.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #155 +/- ##
=======================================
Coverage 95.42% 95.42%
=======================================
Files 32 32
Lines 3037 3037
=======================================
Hits 2898 2898
Misses 139 139 ☔ View full report in Codecov by Sentry. |
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 looking good. If you want to verify more of the functionality, I'd go through the primitives in roughly the order I did (you can check the commit history in cesride
for the PR links I believe) and add tests and necessary wrappers.
} | ||
|
||
const raw = new TextEncoder().encode(JSON.stringify(icp)) | ||
const serder = cesride.Serder.new_with_raw(raw) |
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 think I was mistaken making this new_with_raw()
wrapping in the spike. You can probably use new with optional arguments. The *_with_*()
functions in cesride
are convenient for Rust where you need to define all optional arguments.
Another thing to do would be make the web tests run in CI, right now I think it just does a sanity build of wasm. |
@jasoncolburne cool. Those are both things I think I can manage. Do you want me to just add them to this PR for a mega-PR or I can split them out into issues and smaller PRs? Mostly I just focused on the Dater implementation and getting that wasm import to work in this signify-ts PR https://github.com/WebOfTrust/signify-ts/pull/149/files |
Also changed web demo to use getters/setters.
Added a wasm-bindgen-test that runs for the DaterWrapper. Also added wasm-tests to github actions as well as an updated section to the READMEs detailing how to test the wasm crate. Addresses cesride issue WebOfTrust#162
Added a wasm-bindgen-test that runs for the DaterWrapper. Also added wasm-tests to github actions as well as an updated section to the READMEs detailing how to test the wasm crate. Addresses cesride issue WebOfTrust#162
@jasoncolburne @m00sey Umm messed up the git log a bit because this is a mega-PR but d5a6189 addresses #162 |
@jasoncolburne whoops, that'll be the last commit to this branch. In the interests of not having this being a giga-PR that keeps growing and growing I'm gonna take your suggestions in this thread and do a bunch of mini-PRs to the wasm branch in this repo (that should match this PR). Hopefully, that'll help with reviews whenever we get around to wanting to merge this into the main branch. |
Rationale
We'd like to use wasm and FFI with signify-ts rather than having a bunch of cesr implementations. This is the first step of showing that's possible.
Testing
Created tests on dater in the signify-ts PR but left this mainly where @DmitryKuzmenko and @jasoncolburne had things set up before. Testing the wasm-artifacts in this repo is something we'll have to figure out.
For Reviewers
Didn't bump because main crate didn't change (although I will open some issues in regards to differences with keripy)
Cargo.toml