-
Notifications
You must be signed in to change notification settings - Fork 24
Common Signing Objects from nucypher-core
#730
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
Common Signing Objects from nucypher-core
#730
Conversation
… latest changes from `nucypher-core` can be used.
…e to specify their corresponding UserOperation. Convert `UserOperationToSign` to `nucypher-core`:UserOperation when crafting signature request. Use latest types from Porter for sending signature requests and processing signature response. Add/update tests.
2d2ec86 to
bca94ca
Compare
…now includes signer address. Modify Porter response to handle SignatureResponse and not a tuple of signer address and SignatureResponse. Update tests.
…ore object). Add type guard for PackedUserOperationToSign. Add helper methods for converting number values to bigint, and data values to Uint8Array.
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.
Pull Request Overview
This PR migrates from using a published NPM package version of @nucypher/nucypher-core (v0.14.5) to a local link dependency, and refactors UserOperation signing to use native WASM types instead of JSON serialization. The changes improve type safety and leverage the WASM core library's built-in types and serialization.
Key Changes:
- Replace
@nucypher/nucypher-core: ^0.14.5withlink:../nucypher-core/nucypher-core-wasm-bundleracross all packages - Refactor UserOperation/PackedUserOperation handling to use native
UserOperationSignatureRequestandPackedUserOperationSignatureRequestfrom nucypher-core - Add converter functions (
toCoreUserOperation,toCorePackedUserOperation) to bridge between TypeScript types and WASM types - Update test mocks to use
SignatureResponseclass instead of JSON-based signatures - Add validator index parameter to
Validatorconstructor
Reviewed Changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Updated dependency references from NPM version to local link |
| package.json | Changed nucypher-core dependency to local link |
| packages/*/package.json | Updated nucypher-core dependencies across all packages |
| packages/shared/src/types.ts | Replaced UserOperation type with UserOperationToSign and PackedUserOperationToSign, added converter functions |
| packages/shared/src/porter.ts | Refactored signUserOp to use SignatureResponse instead of JSON decoding |
| packages/taco/src/sign.ts | Updated to use native WASM signature request types |
| packages/test-utils/src/utils.ts | Added index parameter to Validator constructor |
| packages/taco/test/*.ts | Updated tests to use new types and serverAggregate.publicKey |
| packages/shared/test/porter.test.ts | Refactored tests to use SignatureResponse objects |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
nucypher-core.nucypher-core.
manumonti
left a comment
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.
LGTM! Nothing to add. Huge work the three PRs 🙌
nucypher-core.nucypher-core
|
Subsumed by #737 |
Type of PR:
Required reviews:
What this does:
Depends on nucypher/nucypher-core#113
Issues fixed/closed:
Why it's needed:
Notes for reviewers:
lynxnodes aren't running the latest changes from Common signing objects fromnucypher-corenucypher#3646. If they did, then current signing flows would fail. My plan is to updatelynxnodes, porter, contracts, latestnucypher/taconpm package all within a single maintenance day - that day is currently undecided.