-
Notifications
You must be signed in to change notification settings - Fork 24
Signing Bug fixes #741
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
Signing Bug fixes #741
Conversation
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 fixes several bugs in the TACo signing implementation, including correcting signature aggregation ordering, fixing property naming inconsistencies, and improving type exports.
- Sorts signatures by signer address before aggregation to ensure deterministic ordering
- Renames
accountGasLimittoaccountGasLimitsthroughout the codebase for API consistency - Makes the
signatureproperty optional onPackedUserOperationToSignand exports signing types from thetacopackage
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/taco/src/sign.ts | Refactored signature aggregation to sort by signer address and updated to use array of signatures instead of object |
| packages/shared/src/types.ts | Renamed accountGasLimit to accountGasLimits and made signature property optional on PackedUserOperationToSign |
| packages/taco/test/taco-sign.test.ts | Updated test fixtures to use accountGasLimits and modified test data to return signatures out of order to verify sorting |
| packages/shared/test/porter.test.ts | Updated comment in test to reflect the corrected property name accountGasLimits |
| packages/taco/src/index.ts | Exported PackedUserOperationToSign and UserOperationToSign types for external consumption |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## signing-epic #741 +/- ##
===============================================
Coverage ? 89.75%
===============================================
Files ? 97
Lines ? 8451
Branches ? 288
===============================================
Hits ? 7585
Misses ? 863
Partials ? 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ignatures returned from nodes.
…dUserOperationToSign.
…hin `nucypher/taco` package.
799ea26 to
d09fad7
Compare
|
Now that |
Type of PR:
Required reviews:
What this does:
Fixed issues encountered while doing real world testing.
accountGasLimits(proper name) instead ofaccountGasLimitsignatureproperty onPackedUserOperationToSignPackedUserOperationToSignandUserOperationToSignas part ofnucypher/taco(sonucypher/sharedisn't needed).Issues fixed/closed:
Why it's needed:
Notes for reviewers: