feat(clearnode): ERC-1271 smart contract wallet authentication#534
feat(clearnode): ERC-1271 smart contract wallet authentication#534LevanIlashvili wants to merge 2 commits intoerc7824:mainfrom
Conversation
Add support for ERC-1271 (isValidSignature) as a fallback authentication method when ECDSA signature recovery fails or the recovered address doesn't match the claimed wallet address. This enables smart contract wallets (e.g. Account Kit LightAccount, Safe, etc.) to authenticate with ClearNode. Changes: - New erc1271.go: IsContract() and VerifyERC1271Signature() helpers - signer.go: Extract ComputeAuthTypedDataHash() for reuse across both ECDSA and ERC-1271 verification paths - rpc_router_auth.go: ERC-1271 fallback in handleAuthSigVerify() - copies signature before ECDSA recovery to preserve original bytes, iterates configured chains to find and verify the contract wallet - rpc_router.go: Add EthClients field to RPCRouter - main.go: Initialize ethClients from configured blockchain RPCs
Summary of ChangesHello @LevanIlashvili, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances ClearNode's authentication capabilities by introducing support for ERC-1271 smart contract wallets. It establishes a robust fallback mechanism where, if traditional ECDSA signature recovery fails, the system attempts to verify the signature using the ERC-1271 standard. This change broadens compatibility for various smart contract wallets, allowing them to authenticate seamlessly without requiring any new RPC configurations, as it intelligently reuses existing blockchain connections. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthroughAdds ERC‑1271 smart‑contract wallet signature verification and wiring. Introduces an ERC1271Verifier interface, contract detection, and signature validation; extracts EIP‑712 hash computation; updates auth flow to try ECDSA recovery first, then fall back to cross‑chain ERC‑1271 checks. Changes
Sequence DiagramsequenceDiagram
actor Client
participant Router as RPCRouter
participant Signer as Signer
participant ECDSA as ECDSA Recovery
participant ERC1271 as ERC‑1271 Verifier
participant Chain as Chain Contract
Client->>Router: handleAuthSigVerify(params, signature)
Router->>Signer: ComputeAuthTypedDataHash(...)
Signer-->>Router: typed_data_hash
Router->>ECDSA: RecoverAddressFromEip712Signature(hash, sig)
alt ECDSA succeeds & address matches
ECDSA-->>Router: recovered_address
Router-->>Client: verification_passed
else ECDSA fails or mismatch
ECDSA-->>Router: error/mismatch
Router->>ERC1271: iterate EthClients -> VerifyERC1271Signature(contractAddr, hash, sig)
ERC1271->>Chain: Call isValidSignature(hash, signature)
alt returns magic value
Chain-->>ERC1271: magic_value
ERC1271-->>Router: verification_passed
Router-->>Client: verification_passed (contract wallet)
else no magic value
Chain-->>ERC1271: no_magic
ERC1271-->>Router: continue
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for ERC-1271 smart contract wallet authentication, which is a great feature for improving user experience with smart wallets. The implementation looks solid, with a fallback mechanism that attempts standard ECDSA recovery first and then tries ERC-1271 verification across all configured chains. The code is well-structured, especially the refactoring in signer.go to reuse the typed data hash computation.
I have a couple of suggestions for improvement, mainly focused on maintainability and debuggability. One is to use the standard go-ethereum/accounts/abi package for ABI encoding to make the code more robust. The other is to add logging for a potential error case to aid in debugging. Details are in the specific comments.
| func VerifyERC1271Signature(ctx context.Context, client ERC1271Verifier, contractAddr common.Address, hash []byte, signature []byte) (bool, error) { | ||
| // ABI-encode the call: isValidSignature(bytes32 hash, bytes signature) | ||
| // Selector (4 bytes) + hash (32 bytes padded) + offset to bytes (32 bytes) + length (32 bytes) + signature data (padded to 32) | ||
| callData := make([]byte, 0, 4+32+32+32+len(signature)+32) | ||
|
|
||
| // Function selector | ||
| callData = append(callData, isValidSignatureSelector...) | ||
|
|
||
| // bytes32 hash (already 32 bytes) | ||
| if len(hash) != 32 { | ||
| return false, fmt.Errorf("hash must be 32 bytes, got %d", len(hash)) | ||
| } | ||
| callData = append(callData, hash...) | ||
|
|
||
| // Offset to bytes parameter (always 64 = 0x40 for two fixed params) | ||
| offset := make([]byte, 32) | ||
| offset[31] = 64 | ||
| callData = append(callData, offset...) | ||
|
|
||
| // Length of signature bytes | ||
| sigLen := make([]byte, 32) | ||
| bigLen := big.NewInt(int64(len(signature))) | ||
| bigLen.FillBytes(sigLen) | ||
| callData = append(callData, sigLen...) | ||
|
|
||
| // Signature data (padded to 32-byte boundary) | ||
| callData = append(callData, signature...) | ||
| if pad := len(signature) % 32; pad != 0 { | ||
| callData = append(callData, make([]byte, 32-pad)...) | ||
| } |
There was a problem hiding this comment.
The manual ABI encoding for the isValidSignature call is complex and can be error-prone. For better robustness and maintainability, I recommend using the go-ethereum/accounts/abi package. This is the standard library for this purpose in Go and would make the code simpler and less likely to contain subtle bugs related to encoding.
| if cErr != nil || !isContract { | ||
| continue | ||
| } |
There was a problem hiding this comment.
When IsContract returns an error (cErr != nil), it is currently ignored and the loop continues to the next chain. This could hide persistent issues with a specific chain's RPC endpoint, making debugging difficult. I recommend adding a debug log to record the error when it occurs before continuing.
if cErr != nil {
logger.Debug("ERC-1271: IsContract check failed", "chainID", chainID, "address", expectedAddr, "error", cErr)
continue
}
if !isContract {
continue
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@clearnode/main.go`:
- Around line 65-75: The ethclient.Client instances created into the ethClients
map are never closed, leaking RPC connections; update the shutdown path in main
(after the RPC server shutdown block where the process handles termination) to
iterate over ethClients and call Close() on each client, logging the closure
(reference ethClients map and ethclient.Client.Close) and skipping nil entries
to ensure all dialed Ethereum RPC clients are properly closed on shutdown.
In `@clearnode/rpc_router_auth.go`:
- Around line 231-247: The code silently ignores a non-nil hashErr and skips
ERC-1271 verification; before the existing if hashErr == nil block (and before
returning "invalid signature"), log the hash error (include hashErr and any
context like challenge.Address) so operators can diagnose
ComputeAuthTypedDataHash failures — update the code that calls
ComputeAuthTypedDataHash (referenced by typedDataHash and hashErr) to emit a
logger.Warn or logger.Error when hashErr != nil, then fall through as before.
🧹 Nitpick comments (6)
clearnode/rpc_router.go (1)
29-29: Consider initializingEthClientsvia the constructor or a functional option.
EthClientsis set externally inmain.goafterNewRPCRouterreturns. This two-step initialization means the router is briefly in an incomplete state and the field's requirement isn't visible from the constructor signature. Passing it as a parameter (or initializing to an empty map insideNewRPCRouter) would make the dependency explicit and prevent a nil-map surprise if a future caller forgets the assignment.Not blocking — the current code is safe because the
rangeover a nil map is a no-op.clearnode/main.go (1)
67-74: No dial timeout — startup can hang if an RPC endpoint is unreachable.
ethclient.Dialusescontext.Background()internally, so a non-responsive endpoint will block indefinitely. Consider usingethclient.DialContextwith a timeout to keep startup predictable.🛠️ Proposed fix
for chainID, blockchain := range config.blockchains { - client, err := ethclient.Dial(blockchain.BlockchainRPC) + dialCtx, dialCancel := context.WithTimeout(context.Background(), 10*time.Second) + client, err := ethclient.DialContext(dialCtx, blockchain.BlockchainRPC) + dialCancel() if err != nil {clearnode/rpc_router_auth.go (1)
233-246: Cross-chain ERC-1271 scan: consider operational implications.Each failed ECDSA auth attempt triggers up to 2 RPC calls per configured chain (CodeAt + CallContract). With many chains, this amplifies latency and RPC load on every invalid EOA signature. This is likely acceptable for a small number of chains, but worth keeping in mind as chains are added.
A few mitigations to consider as the chain count grows:
- Allow the client to specify a
chain_idhint in the auth request to try that chain first.- Add a context deadline/timeout specifically for the ERC-1271 verification loop.
clearnode/signer.go (1)
133-165: Note:RecoverAddressFromEip712Signaturestill mutates the inputsigslice.Line 152-153 modifies
sig[64]in place. The caller inrpc_router_auth.gocorrectly copies the signature before calling this function, but this mutation is a latent footgun for future callers. Consider documenting this in the function's godoc, or defensively copying inside the function.This is pre-existing behavior, not introduced by this PR — just calling it out for awareness.
clearnode/erc1271.go (2)
13-18:erc1271MagicValueandisValidSignatureSelectorare identical by spec — unify them.Per ERC-1271, the magic return value is the function selector (
bytes4(keccak256("isValidSignature(bytes32,bytes)"))). Having two separate definitions — one hardcoded, one computed — creates a subtle maintenance risk if either is updated independently.♻️ Proposed simplification
-// ERC-1271 magic value returned by isValidSignature when the signature is valid. -// bytes4(keccak256("isValidSignature(bytes32,bytes)")) -var erc1271MagicValue = [4]byte{0x16, 0x26, 0xba, 0x7e} - -// isValidSignature(bytes32,bytes) selector -var isValidSignatureSelector = crypto.Keccak256([]byte("isValidSignature(bytes32,bytes)"))[:4] +// isValidSignatureSelector is the 4-byte function selector for +// isValidSignature(bytes32,bytes). Per ERC-1271, this is also the +// magic value returned on success. +var isValidSignatureSelector = crypto.Keccak256([]byte("isValidSignature(bytes32,bytes)"))[:4]Then at line 81, compare against
isValidSignatureSelector:- return result[0] == erc1271MagicValue[0] && - result[1] == erc1271MagicValue[1] && - result[2] == erc1271MagicValue[2] && - result[3] == erc1271MagicValue[3], nil + return result[0] == isValidSignatureSelector[0] && + result[1] == isValidSignatureSelector[1] && + result[2] == isValidSignatureSelector[2] && + result[3] == isValidSignatureSelector[3], nil
37-37: Consider usinggo-ethereum/accounts/abifor ABI encoding.The manual encoding is correct for this specific call, but using the
abipackage would provide encoding validation and make it easier to adapt if the interface changes. This is optional — the manual approach is fine for a stable, simple ABI like ERC-1271.
|
@LevanIlashvili , thank you for this PR! |
2 participants
Summary
isValidSignature) support as a fallback when ECDSA signature recovery fails or the recovered address doesn't match the claimed walletHow it works
handleAuthSigVerifyfirst attempts standard ECDSA recoveryeth_getCode)isValidSignature(bytes32,bytes)on the contractFiles changed
clearnode/erc1271.goIsContract()+VerifyERC1271Signature()helpersclearnode/signer.goComputeAuthTypedDataHash()for reuse in both ECDSA and ERC-1271 pathsclearnode/rpc_router_auth.gohandleAuthSigVerify()clearnode/rpc_router.goEthClients map[uint32]*ethclient.Clientfieldclearnode/main.goethClientsfrom configured blockchain RPCsTest plan
go build ./...passesSummary by CodeRabbit
New Features
Improvements