-
Notifications
You must be signed in to change notification settings - Fork 8
build: updates rust version and WASM crates to latest versions #19
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
build: updates rust version and WASM crates to latest versions #19
Conversation
1bff5a7 to
9ece847
Compare
|
The build failed, and by some investigation, I found the reason to be the github action https://github.com/jetli/wasm-pack-action was using version 'v0.9.1'. |
|
Now, the tests fail with error I'm guessing that's because of a memory leak indeed. I don't notice this on my local machine, but I'll try to improve the tests anyway, and see if that makes a difference. |
4f0b5ca to
fd85963
Compare
|
Update: Rerunning the tests with no code updates succeeded with no issues. I'll leave this issue for now, and we can come back to this problem later after the final update of the libraries. |
fd85963 to
b4e6710
Compare
Cargo.toml
Outdated
| wasm-bindgen = "= 0.2.100" | ||
| dlmalloc = { version = "0.2.6", features = ["global"], optional = true } | ||
| serde_with = { version = "1.10.0", default-features = false, features = ["macros"] } | ||
| serde_with = { version = "3.14.0", default-features = false, features = ["macros"] } |
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.
Upgrading this caused the serialization to change which is a breaking change so we need adapt in the TS lib (these tests specifically). The old serialized data used in those tests is here.
Is there a feature that you want from the upgraded version of this dependency?
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.
Upgrading this .... The old serialized data used in those tests is here.
Oh! It didn't come to mind to look at the crypto-wasm-ts until I finished the updates here. Now, I'm thinking maybe I need to start running the crypto-wasm-ts tests on this library today.
Is there a feature that you want from the upgraded version of this dependency?
Not really! I was just worried that it's lagging behind with 2 major versions. However, since there's no incompatibility problems raised by this so far, I can just postpone until later to avoid having many issues to solve in crypto-wasm-ts at the same time.
Cargo.toml
Outdated
| serde_json = { version = "1.0"} | ||
| serde-wasm-bindgen = "0.6.5" | ||
| wasm-bindgen = "= 0.2.86" | ||
| wasm-bindgen = "= 0.2.100" |
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.
Similar concern as with the serde_with dependency, i think upgrading it changed some serialisation but I am not that confident of my memory here. (Should have put a note for the reason of pinning it).
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.
Here, this is a bit tricky as this update is needed for newer versions of rust, and also to avoid conflicting dependencies with the latest version of the crypto crates. (I've already tried updating the dock crypto crates to the latest version but this pinned version was a problem.)
I'm thinking about trying to load this library into crypto-wasm-ts and run the tests to make sure they succeed, then we can proceed on this. What do you think?
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'm thinking about trying to load this library into crypto-wasm-ts and run the tests to make sure they succeed, then we can proceed on this. What do you think?
Yes, for bigger upgrades, i run crypto-wasm-ts with crypto-wasm before publishing crypto-wasm itself. I make crypto-wasm a local dependency of crypto-wasm-ts. For some reason, yarn link sometimes didn't work so I had to manually remove the dependency's directory from node_modules when I updated the dependency (crypto-wasm here)
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 struggled with that a bit yesterday, until I found that new versions of yarn introduced the portal keyword, and it works like this (in your package.json file).
"crypto-wasm-new": "portal:../crypto-wasm",This works without issues for me so far.
The link command was removed in newer versions of yarn.
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.
Hello Lovesh, I've worked on the crypto-wasm-ts to make sure the changes here are good (as we said above).
Now, I have this PR with the necessary changes to make everything work.
The necessary changes are in this PR docknetwork/crypto-wasm-ts#43
Note that in one commit docknetwork/crypto-wasm-ts@bed0d12 I had to remove a bunch of tests because we've removed the old proof-system dependency which means this support is gone now.
Does this look okay to you?
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.
@lovesh I updated the versions here to:
wasm-bindgen = "0.2.95" // first working version compatible with everything else
// serde_with is reverted to initial version
serde_with = { version = "1.10.0", default-features = false, features = ["macros"] }
And I've tested this with crypt-wsm-ts by running the tests over there.
The one feature I removed there was supporting presentation versions older than 0.10 as they required the legacy version of the proof-system library.
Also, I've pushed this commit which allows you to run the tests on crypto-wasm-ts using a local version of crypto-wasm (as you know - but portal syntax might be new).
This is of course only for testing and won't be merged. We will update the other PR once we get this one merged.
What do you think?
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.
btw, I deleted the tests for the old presentation versions, but an alternative solution is to keep them like this
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.
supporting presentation versions older than 0.10
That would mean supporting only 0.10. But maybe its fine since 0.10 has been out for so long.
but an alternative solution is to keep them like this
I didn't follow the suggestion. You mean keep supporting it
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.
You mean keep supporting it?
@lovesh No.
In my first PR, I just deleted the tests that used old presentation versions.
In the other approach I keep the tests, but change all the presentation versions to v10.
The value I see here is make sure that older versions of credentials still work well with newer versions of presentations. However, I could be wrong!
If you don't have a preference, We can just go with the first solution and remove the tests.
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.
btw, can we have a new release here now? so we can update crypto-wasm-ts to use it? Thank you!
Description
With the merge of the last PR #17, the project is now ready to run with Rust 1.89 (latest stable version as of the time of writing).
In the PR, I update Rust version, and also update all the WASM libraries to the latest versions.
Notice the change in the script file
scripts/build-package.shwhere this change arises from a very subtle change in the workings of the librarywasm-bindgen.This encourages a rethink on this part of the build script to avoid future bugs.
Next steps
In the following PR, I'll update the docknetwork crypto libraries to the latest versions. In fact, this PR is a prerequisite to updating the libraries as the dependencies conflict otherwise.
PR checklist
yarn install; yarn build; yarn testto make sure everything runs as before.Does this PR introduce a breaking change?
Which merge strategy will you use?