chore: update api remove callback #53
Conversation
…VerifierTimedSignature
WalkthroughThe changes in this pull request include updates to the Continuous Integration (CI) workflow to trigger on the Changes
Possibly related issues
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (12)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🧰 Additional context used🪛 Markdownlint
🔇 Additional comments (33)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (17)
.github/workflows/ci.yaml (1)
5-5: LGTM: CI workflow now includes thedevbranchThe addition of the
devbranch to bothpushandpull_requestevents is a good practice. This change ensures that the CI pipeline runs for changes in both themainanddevbranches, which can help catch issues earlier in the development process.Consider the following suggestions to further improve your CI workflow:
- If you have a staging environment, you might want to add automatic deployments for the
devbranch.- Implement branch protection rules for both
mainanddevbranches to ensure CI passes before merging.- If not already in place, consider adding a step to your CI that creates and uploads build artifacts for easy testing and deployment.
Also applies to: 7-7
Makefile (1)
17-18: LGTM: New target for additional Solidity build outputs.The new
build-solidity-extratarget is a valuable addition, generating ABI and binary files alongside the standard build. This is likely useful for other parts of the project, such as generating Go bindings.Consider adding a comment explaining the purpose and use cases for this new target to improve maintainability.
test/integration/auth_test.go (1)
Further Action Required: Inconsistent
TimedSignatureUsage DetectedWhile the change in
test/integration/auth_test.gocorrectly updates thesigvariable type from*server.TimedSignatureto*tas.SignatureVerifierTimedSignature, the search results indicate thatserver.TimedSignatureis still used in multiple other files throughout the codebase.
scripts/e2e/main.gosrc/go/server/server_test.gosrc/go/contract/transferable_account_store/transferable_account_store.goPlease ensure that all instances of
server.TimedSignatureare reviewed and updated as necessary to maintain consistency and prevent potential compatibility issues.🔗 Analysis chain
Line range hint
85-90: LGTM: Variable type updated correctly.The change from
*server.TimedSignatureto*tas.SignatureVerifierTimedSignaturealigns with the new import and likely reflects updates in the underlying implementation.To ensure consistency across the codebase, please run the following script to check for any other occurrences of
TimedSignaturethat might need updating:Please review the results to determine if any other files need to be updated to use the new
tas.SignatureVerifierTimedSignaturetype.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for other occurrences of TimedSignature in the codebase # Test: Search for TimedSignature usage echo "Searching for TimedSignature usage:" rg --type go "TimedSignature" # Test: Search for server.TimedSignature usage specifically echo "Searching for server.TimedSignature usage:" rg --type go "server\.TimedSignature"Length of output: 30126
src/solidity/interfaces/ITransferableAccountStore.sol (2)
13-13: LGTM. Consider updating documentation.The change from
Curve curvetoSignatureAlgorithm signatureAlgorithmin theAccountstruct is consistent with the overall refactoring. This modification improves clarity by explicitly stating that it's a signature algorithm rather than a generic curve.Consider updating any relevant documentation or comments to reflect this change in terminology from "curve" to "signature algorithm".
17-20: LGTM. Consider adding a comment for UNSPECIFIED.The addition of the
SignatureAlgorithmenum is well-structured and aligns with the changes in theAccountstruct. The naming convention is clear and consistent.Consider adding a brief comment explaining the purpose of the
SignatureAlgorithm_UNSPECIFIEDvalue. For example:enum SignatureAlgorithm { SignatureAlgorithm_UNSPECIFIED, // Default value, indicates no algorithm set SignatureAlgorithm_ECDSA, SignatureAlgorithm_EDDSA }src/proto/api/v1/transferable_account.proto (2)
19-22: Approval: NewSignatureAlgorithmenum addedThe addition of the
SignatureAlgorithmenum is a good improvement. It provides a clear and extensible way to specify signature algorithms, which aligns well with the change in theAccountmessage.Positive aspects:
- Follows Protocol Buffers naming conventions.
- Includes an UNSPECIFIED (0) value, which is a good practice.
- Covers common signature algorithms (ECDSA and EDDSA).
Suggestion for improvement:
Consider adding a comment to describe each enum value, especially explaining the difference between ECDSA and EDDSA for clarity. For example:enum SignatureAlgorithm { SignatureAlgorithm_UNSPECIFIED = 0; // Default value when not set SignatureAlgorithm_ECDSA = 1; // Elliptic Curve Digital Signature Algorithm SignatureAlgorithm_EDDSA = 2; // Edwards-curve Digital Signature Algorithm }
Line range hint
1-190: Summary: API updated to use SignatureAlgorithmThe changes in this file successfully update the API to use a more flexible
SignatureAlgorithminstead of the previousCurve. This aligns well with the PR objective of updating the API and removing the callback.Key points:
- The
Accountmessage now usesSignatureAlgorithminstead ofCurve.- A new
SignatureAlgorithmenum has been added with appropriate values.Recommendations:
- Ensure all client code is updated to use the new
signature_algorithmfield.- Update any data storage or serialization code to handle the new format.
- Revise API documentation to reflect these changes.
- Consider adding comments to the
SignatureAlgorithmenum for clarity.- Implement and test a migration strategy for existing data.
Overall, these changes improve the API's flexibility and clarity. However, careful attention must be paid to maintaining backwards compatibility or providing a clear upgrade path for existing users of the API.
docs/api.md (1)
34-34: Approve change and fix indentationThe addition of 'SignatureAlgorithm' to the table of contents is correct and consistent with the API changes. However, the indentation needs to be adjusted.
Please fix the indentation of this line to match the other entries in the list. The expected indentation is 2 spaces, but the actual indentation is 4 spaces. Update the line as follows:
- - [SignatureAlgorithm](#api-v1-SignatureAlgorithm) + - [SignatureAlgorithm](#api-v1-SignatureAlgorithm)🧰 Tools
🪛 Markdownlint
34-34: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
test/TransferableAccountStore.t.sol (2)
Line range hint
111-123: LGTM! Consider clarifying the assertion message.The change from
CurvetoSignatureAlgorithmis consistent and aligns with the updated data structure. This modification improves the clarity of the account's signature verification method.Consider updating the assertion message to be more specific:
- assertEq( - uint256(signatureAlgorithm), uint256(decodedAccount.signatureAlgorithm), "Stored account curve should match" - ); + assertEq( + uint256(signatureAlgorithm), uint256(decodedAccount.signatureAlgorithm), "Stored account signature algorithm should match" + );
220-222: LGTM! Consider updating the assertion message.The change from
CurvetoSignatureAlgorithmin the assertion is consistent with the earlier modifications and reflects the updated data structure.Consider updating the assertion message to be more specific:
assertEq( - uint256(retrievedAccount.signatureAlgorithm), uint256(account.signatureAlgorithm), "Curve should match" + uint256(retrievedAccount.signatureAlgorithm), uint256(account.signatureAlgorithm), "Signature algorithm should match" );src/solidity/TransferableAccountStore.sol (3)
116-118: Access Control EnhancementThe access control check uses
isOwnerto verify if the signer is the owner. Ensure that this function reliably determines ownership and consider emitting an event or logging when access is denied for better traceability.
119-120: Potential Optimization in Account ID ConversionThe use of
Utils.iToHex(abi.encodePacked(accountId))may introduce unnecessary overhead. IfaccountIdis already in the desired format, consider removing redundant conversions to optimize gas usage.Apply this diff if appropriate:
-emit AddressApproved(Utils.iToHex(abi.encodePacked(accountId)), _address); +emit AddressApproved(accountId, _address);
244-246: Consistent Error MessagesFor clarity and consistency, ensure that all error messages and event emissions use the same format and provide sufficient information for debugging. This might involve standardizing the message content or structure.
src/go/server/server.go (4)
70-105: Ensure secure handling of private keys.In
NewServer, theprivateKeyis passed as a string and used to create the Ethereum transactor. Storing and passing private keys as plain strings can pose security risks. Consider using secure methods for managing private keys, such as a secrets manager or integrating with a secure key storage service. Ensure that the private key is not logged or exposed in error messages.
123-132: Variable namegprcsmay be a typo; consider renaming for clarity.The variable
gprcsseems to be a typo or may cause confusion. Consider renaming it togrpcServerorserverto improve readability.Apply this diff to rename the variable:
- gprcs := grpc.NewServer() + grpcServer := grpc.NewServer()And update all references accordingly:
- pb.RegisterAccountServiceServer(gprcs, s) + pb.RegisterAccountServiceServer(grpcServer, s) ... - if err := gprcs.Serve(lis); err != nil { + if err := grpcServer.Serve(lis); err != nil {
195-195: Improve error logging for better context.The error is logged as
err: %v, which lacks context. Consider adding more descriptive messages to aid in debugging. Include the function name and a brief description of the operation.Apply this diff:
-log.Printf("err: %v", err) +log.Printf("TransferAccount: failed to populate timed signature: %v", err)
421-435: Include transaction hash in error message for failed transactions.When a transaction fails, including the transaction hash in the error message can help with debugging and tracking issues.
Apply this diff:
if receipt.Status != types.ReceiptStatusSuccessful { - return nil, fmt.Errorf("transaction failed") + return nil, fmt.Errorf("transaction %s failed", tx.Hash().Hex()) }
🛑 Comments failed to post (9)
src/solidity/TransferableAccountStore.sol (6)
167-169: 🛠️ Refactor suggestion
Duplicate Signature Verification Logic
The signature verification logic is duplicated here. Refer to the earlier suggestion about creating a modifier to handle this check uniformly across all functions.
259-264: 🛠️ Refactor suggestion
Eliminate Redundant Signature Verification
As previously suggested, refactor the signature verification into a modifier to streamline the code and enhance readability.
283-291:
⚠️ Potential issueCorrect Error Name for Locked Account
In the
signfunction, the errorOnlyUnlockAccountis raised whenisAccountLocked(accountId)returns true. This might be confusing due to the naming. Consider renaming the error toAccountLockedorAccountIsLockedfor better clarity.Apply this diff to rename the error and update its usage:
error OnlyApprovedAccount(); -error OnlyUnlockAccount(); +error AccountLocked(); ... if (isAccountLocked(accountId)) { - revert OnlyUnlockAccount(); + revert AccountLocked(); }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if (!verifyTimedSignature(timedSignature)) { revert InvalidTimedSignature(); } if (!isApproved(accountId, timedSignature.signer)) { revert OnlyApprovedAccount(); } if (isAccountLocked(accountId)) { revert AccountLocked(); }
42-48: 🛠️ Refactor suggestion
Consistent Error Naming for Clarity
Consider renaming the
OnlyUnlockAccounterror toOnlyUnlockedAccountfor consistency with the naming convention used in other error definitions likeOnlyOwnerCanApproveAddressesandOnlyOwnerCanUnlockAccount. This enhances readability and consistency throughout the contract.Apply this diff to rename the error:
error InvalidTimedSignature(); error OnlyOwnerCanApproveAddresses(); error OnlyOwnerCanRevokeApproval(); error OnlyOwnerCanDeleteAccount(); error OnlyOwnerCanUnlockAccount(); error OnlyApprovedAccount(); -error OnlyUnlockAccount(); +error OnlyUnlockedAccount();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.error InvalidTimedSignature(); error OnlyOwnerCanApproveAddresses(); error OnlyOwnerCanRevokeApproval(); error OnlyOwnerCanDeleteAccount(); error OnlyOwnerCanUnlockAccount(); error OnlyApprovedAccount(); error OnlyUnlockedAccount();
112-114: 🛠️ Refactor suggestion
Consolidate Repeated Signature Verification
The check for
!verifyTimedSignature(timedSignature)and subsequent revert is repeated across multiple functions. Consider creating a modifier or a private function to handle this verification to reduce code duplication and improve maintainability.Apply this diff to create a modifier for signature verification:
+modifier verifySignature(SignatureVerifier.TimedSignature calldata timedSignature) { + if (!verifyTimedSignature(timedSignature)) { + revert InvalidTimedSignature(); + } + _; +} function approveAddress( SignatureVerifier.TimedSignature calldata timedSignature, string memory accountId, address _address -) external onlyLocked(accountId) { - if (!verifyTimedSignature(timedSignature)) { - revert InvalidTimedSignature(); - } +) + external + onlyLocked(accountId) + verifySignature(timedSignature) +{ Account storage account = accountsStore[accountId]; if (!isOwner(accountId, timedSignature.signer)) { revert OnlyOwnerCanApproveAddresses(); } accountApprovals[account.accountId] = _address; emit AddressApproved(Utils.iToHex(abi.encodePacked(accountId)), _address); }Repeat this pattern for other functions where signature verification is performed.
Committable suggestion was skipped due to low confidence.
217-227: 🛠️ Refactor suggestion
Refactor Access Checks and Error Handling
The
transferAccountfunction includes repeated access checks and error handling similar to other functions. Implement the suggested signature verification modifier and consider adding an access control modifier for approved accounts to reduce redundancy.Apply this diff to create an
onlyApprovedmodifier:+modifier onlyApproved(string memory accountId, address signer) { + if (!isApproved(accountId, signer)) { + revert OnlyApprovedAccount(); + } + _; +} function transferAccount( SignatureVerifier.TimedSignature calldata timedSignature, string memory accountId, address to -) public onlyLocked(accountId) { - if (!verifyTimedSignature(timedSignature)) { - revert InvalidTimedSignature(); - } - if (!isApproved(accountId, timedSignature.signer)) { - revert OnlyApprovedAccount(); - } +) + public + onlyLocked(accountId) + verifySignature(timedSignature) + onlyApproved(accountId, timedSignature.signer) +{ Account storage account = accountsStore[accountId]; account.owner = to; delete accountApprovals[account.accountId]; emit AccountTransferred(accountId, account); }Committable suggestion was skipped due to low confidence.
src/go/server/server_test.go (3)
24-27:
⚠️ Potential issueAvoid using global variables in tests to ensure test isolation and prevent potential data races
Declaring
sas a global variable can lead to concurrency issues when tests are run in parallel. Each test should have its own instance ofserverto maintain isolation and prevent shared state.
25-25:
⚠️ Potential issueAvoid using global variables to store state in tests
Using
accountIdas a global variable can cause tests to interfere with each other, especially if tests are executed concurrently. Consider returningaccountIdfromnewAccountand passing it explicitly to test functions.
604-645: 🛠️ Refactor suggestion
Refactor helper functions to return errors instead of calling
t.FatalfCalling
t.Fatalfwithin helper functions can obscure the source of errors and halt the entire test suite. Instead, have helper functions return errors and handle them appropriately in the test functions.Apply this diff to modify the helper functions:
-func newAccount(t *testing.T, privateKey *ecdsa.PrivateKey) *pb.Account { +func newAccount(privateKey *ecdsa.PrivateKey) (*pb.Account, error) { sig := newTimedSignature(privateKey) receipt, err := s.taStoreContract.SendConfidentialRequest("createAccount", []interface{}{sig}, nil) if err != nil { + return nil, fmt.Errorf("failed to send confidential request: %v", err) } ev, err := s.taStoreContract.Abi.Events["AccountCreated"].ParseLog(receipt.Logs[0]) if err != nil { + return nil, fmt.Errorf("failed to parse log: %v", err) } accountId := ev["accountId"].(string) return &pb.Account{ AccountId: accountId, Owner: crypto.PubkeyToAddress(privateKey.PublicKey).Hex(), }, nil }Update the calls to
newAccountin your tests to handle the returned error:-account := newAccount(t, privateKey) +account, err := newAccount(privateKey) +if err != nil { + t.Fatalf("failed to create new account: %v", err) +}Committable suggestion was skipped due to low confidence.
related:
I tested with:
also don't forget to run:
Summary by CodeRabbit
Release Notes
New Features
devbranch.Improvements
Bug Fixes
Chores
.gitignoreto include new directories for ignored files.