test/utils/helpers.go (2)

Line range hint 35-57: Remove redundant error checks

The function has redundant assertions after proper error handling is already in place. This creates confusion about which error handling approach should be followed.

Remove the redundant assertions:

 func ApproveAddressHelper(t *testing.T, taStoreContract *framework.Contract, accountId string, ownerPrivKey *ecdsa.PrivateKey, addressToApprove string) error {
     validFor := uint64(time.Now().AddDate(1, 0, 0).Unix())
     approveSig, err := NewPbTimedSignature(taStoreContract, ownerPrivKey, validFor, common.HexToHash(impl.APPROVE_ADDRESS_FUNCTION_HASH))
     if err != nil {
         return fmt.Errorf("failed to generate timed signature: %w", err)
     }
     approveAddressRequest := &pb.ApproveAddressRequest{
         Base: &pb.AccountOperationRequest{
             AccountId: accountId,
             Proof:     approveSig,
         },
         Address: addressToApprove,
     }
     _, resp, err := ApproveAddress(approveAddressRequest)
-    assert.NoError(t, err, "Failed to approve address")
-    assert.Equal(t, 200, resp.StatusCode)
     if err != nil {
         return fmt.Errorf("failed to approve address: %w", err)
     }
     if resp.StatusCode != 200 {
         return fmt.Errorf("unexpected status code: %d", resp.StatusCode)
     }
     return nil
 }

---

Line range hint 61-82: Remove redundant error checks

Similar to ApproveAddressHelper, this function has redundant assertions after proper error handling is already in place.

Remove the redundant assertions:

 func UnlockAccountHelper(t *testing.T, taStoreContract *framework.Contract, accountId string, ownerPrivKey *ecdsa.PrivateKey) error {
     validFor := uint64(time.Now().AddDate(1, 0, 0).Unix())
     unlockSig, err := NewPbTimedSignature(taStoreContract, ownerPrivKey, validFor, common.HexToHash(impl.UNLOCK_ACCOUNT_FUNCTION_HASH))
     if err != nil {
         return fmt.Errorf("failed to generate timed signature: %w", err)
     }
     unlockAccountRequest := &pb.UnlockAccountRequest{
         Base: &pb.AccountOperationRequest{
             AccountId: accountId,
             Proof:     unlockSig,
         },
     }
     _, resp, err := UnlockAccount(unlockAccountRequest)
     if err != nil {
         return fmt.Errorf("failed to unlock account: %w", err)
     }
     if resp.StatusCode != 200 {
         return fmt.Errorf("unexpected status code: %d", resp.StatusCode)
     }
-    assert.NoError(t, err, "Failed to unlock account")
-    assert.Equal(t, 200, resp.StatusCode)
     return nil
 }

test/e2e/e2e_test.go (2)

437-440: LGTM! Consistent error handling for address approval.

The error handling for ApproveAddressHelper is well-implemented with clear error messages.

Consider adding error scenario test cases.

While the happy path is well tested, consider adding test cases that verify the error handling behavior of these helper functions (e.g., network errors, invalid inputs).

Example test case:

{
    name: "Invalid address format",
    setupFunc: func() (string, string) {
        accountId, err := testutil.CreateAccountHelper(t, taStoreContract, alicePrivKey)
        if err != nil {
            t.Fatalf("Failed to create account: %v", err)
        }
        return accountId, "invalid_address_format"
    },
    expectValid: false,
},

Also applies to: 633-636

---

Line range hint 1-784: Consider documenting helper function contracts.

The test file makes extensive use of helper functions (CreateAccountHelper, UnlockAccountHelper, ApproveAddressHelper). Consider adding documentation comments that describe:

• Expected behavior and return values
• Possible error conditions
• Required preconditions
• Side effects

This would make the test cases more maintainable and easier to understand.

Example:

// CreateAccountHelper creates a new account with the given private key.
// Returns the account ID and any error encountered.
//
// Possible errors:
// - Network errors during contract interaction
// - Invalid private key format
// - Contract execution errors
func CreateAccountHelper(t *testing.T, contract *framework.Contract, privKey *ecdsa.PrivateKey) (string, error)

test/TransferableAccountStore.t.sol (3)

13-14: Consider using test key generation instead of hardcoded private keys.

While converting private keys to constants improves immutability, consider using a deterministic key generation function for test accounts instead of hardcoding private keys, even in tests. This prevents potential misuse if test keys are accidentally used in production.

-    uint256 public constant ALICE_PRIVATE_KEY = 0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa;
-    uint256 public constant BOB_PRIVATE_KEY = 0xbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb;
+    uint256 public constant ALICE_PRIVATE_KEY = uint256(keccak256("test_key_alice"));
+    uint256 public constant BOB_PRIVATE_KEY = uint256(keccak256("test_key_bob"));

---

34-35: Standardize error naming convention.

The error types have inconsistent naming. AccountLockedError includes the 'Error' suffix while AccountLocked doesn't.

-    error AccountLocked();
-    error AccountLockedError();
+    error AccountLockedError();
+    error AccountNotLockedError();

---

Line range hint 37-64: Add input validation to generateTimedSignature helper.

The helper function should validate that validFor is not in the past and signer is not zero address.

     function generateTimedSignature(
         uint64 validFor,
         address signer,
         uint256 privateKey,
         uint64 nonce,
         bytes32 targetFunctionHash
     ) internal pure returns (SignatureVerifier.TimedSignature memory) {
+        require(validFor > block.timestamp, "Invalid validity period");
+        require(signer != address(0), "Invalid signer address");
         bytes32 messageHash = SignatureVerifier.hashMessage(validFor, signer, nonce, targetFunctionHash);

Configuration used: .coderabbit.yaml
Review profile: CHILL

scripts/utils/generate_function_hash/main.go (2)

19-22: Consider enhancing the error message.

While the empty input validation is good, the error message could be more descriptive to help users understand what went wrong and how to fix it.

-		fmt.Fprintln(os.Stderr, "Error: Empty input string")
+		fmt.Fprintln(os.Stderr, "Error: Function signature cannot be empty")

---

Line range hint 1-34: Consider adding batch processing capability.

For improved developer workflow, consider adding support for:

1. Reading multiple function signatures from a file
2. Processing multiple signatures provided as command-line arguments
3. JSON output format option for better integration with other tools

This would make the utility more versatile for processing multiple function signatures in one go.

scripts/utils/generate_timed_signature/main.go (1)

71-73: Consider security improvements for signature generation

The current implementation has several security considerations:

1. The hardcoded 24-hour validity period might be too long for sensitive operations like DeleteAccount or TransferAccount
2. Loading private keys from command line arguments poses a security risk as they could be visible in shell history

Consider these improvements:

1. Make the validity duration configurable based on the operation type
2. Add support for loading private keys from environment variables or secure files
3. Add validation for minimum/maximum validity periods

Would you like me to propose a specific implementation for these security improvements?

Configuration used: .coderabbit.yaml
Review profile: CHILL

#!/bin/bash
# Description: Check for any remaining instances of 'sender' that might need updating
# for consistency with the new 'signer' terminology in signature verification contexts

# Search for 'sender' in Solidity files
rg -t sol 'sender' --context 3

#!/bin/bash
# Let's try a different approach using fd to find Solidity files first
# Then use ripgrep without file type specification

# Find all Solidity files and search for 'sender' in them
fd -e sol | xargs rg 'sender' -C 3

# Also search for the new term 'signer' to understand the context
fd -e sol | xargs rg 'signer' -C 3

# Additionally, let's use ast-grep to find function declarations with 'sender' or 'signer' parameters
ast-grep --pattern 'function $FNAME($$$sender$$$) { $$$ }'
ast-grep --pattern 'function $FNAME($$$signer$$$) { $$$ }'

test/TransferableAccountStore.t.sol (3)

13-14: Consider security implications of public test keys.

While making test private keys public constants improves readability, consider adding a comment to explicitly indicate these are test-only keys to prevent misuse in production.

-    uint256 public constant ALICE_PRIVATE_KEY = 0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa;
-    uint256 public constant BOB_PRIVATE_KEY = 0xbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb;
+    // WARNING: These are test-only keys. Never use in production!
+    uint256 public constant ALICE_PRIVATE_KEY = 0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa;
+    uint256 public constant BOB_PRIVATE_KEY = 0xbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb;

---

34-35: Differentiate error types more clearly.

The error types AccountLocked and AccountLockedError are too similar and might cause confusion. Consider renaming for clarity.

-    error AccountLocked();
-    error AccountLockedError();
+    error AccountIsLocked();
+    error AccountLockStateError();

---

Line range hint 66-502: Consider refactoring common test patterns.

Many test functions share similar setup patterns (creating accounts, generating signatures). Consider extracting these into helper functions to improve maintainability and reduce code duplication.

Example helper function:

function setupTestAccount(
    TransferableAccountStore tas,
    address owner,
    uint256 privateKey
) internal returns (string memory accountId) {
    SignatureVerifier.TimedSignature memory sig = generateTimedSignature(
        uint64(block.timestamp + 86400),
        owner,
        privateKey,
        tas.getNonce(owner),
        CREATE_ACCOUNT_FUNCTION_HASH
    );
    bytes memory encodedData = tas.createAccount(sig);
    bytes memory accountData = decodeEncodedData(encodedData);
    
    (
        SignatureVerifier.TimedSignature memory decodedTimedSignature,
        ITransferableAccountStore.Account memory decodedAccount
    ) = abi.decode(accountData, (SignatureVerifier.TimedSignature, ITransferableAccountStore.Account));
    
    return tas.createAccountCallback(decodedTimedSignature, decodedAccount);
}

Configuration used: .coderabbit.yaml
Review profile: CHILL

-        if (isAccountLocked) {
-            revert AccountLocked();
-        }