Add external sign #844
Add external sign #844
Conversation
📝 WalkthroughWalkthroughL2 client constructor now accepts the full config and supports external signing; a new Signer type implements external-sign transaction creation and signing; CLI flags and config fields for external signing added; tx sending routes to local or external sign paths; externalsign dependency added. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Updater as TxManager
participant Client as L2Client
participant Signer as Signer
participant RPC as Ethereum RPC
participant ExtSvc as ExternalSign Service
Note over Updater,Client: External-sign enabled send flow
Updater->>Client: SendTransaction(to, calldata)
Client->>Signer: CreateAndSignTx(ctx, client, to, calldata)
Signer->>RPC: eth_getTransactionCount (nonce)
Signer->>RPC: eth_maxPriorityFeePerGas (tip)
RPC-->>Signer: tip & header (baseFee)
Signer->>RPC: eth_estimateGas (NoSend simulation)
RPC-->>Signer: gas estimate
Signer-->>Signer: compute fee caps & gas limit (1.5x buffer)
Signer->>ExtSvc: request external signature (unsigned DynamicFeeTx)
ExtSvc-->>Signer: signed raw transaction
Signer-->>Client: signed *types.Transaction
Client->>RPC: eth_sendRawTransaction (signed)
RPC-->>Client: tx hash
Client->>Updater: return tx hash
Updater->>RPC: poll eth_getTransactionReceipt until mined / timeout
RPC-->>Updater: receipt
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
token-price-oracle/client/sign.go (1)
85-97: Consider addingValueparameter support for ETH transfers.The
CreateAndSignTxfunction creates aDynamicFeeTxwithout aValuefield, which means it cannot be used for transactions that need to send ETH. If this is intentional for this oracle use case (contract calls only), consider adding a comment to clarify. Otherwise, add avalue *big.Intparameter.token-price-oracle/flags/flags.go (1)
181-184:PrivateKeyFlagis inrequiredFlagsbut no longer hasRequired: true.The
PrivateKeyFlagwas updated to removeRequired: true(since it's not needed when external signing is enabled), but it's still listed inrequiredFlags. This is misleading for documentation purposes. Consider moving it tooptionalFlagssince the actual validation is handled inconfig.go.🔎 Proposed fix
var requiredFlags = []cli.Flag{ L2EthRPCFlag, - PrivateKeyFlag, } var optionalFlags = []cli.Flag{ + PrivateKeyFlag, TxnPerBatchFlag,token-price-oracle/config/config.go (1)
227-240: Validation logic is correct, but consider reducing information in error messages.The validation correctly enforces that either all external sign fields are provided or the private key is required. However, the error message on lines 232-233 exposes the actual values of
cfg.ExternalSignAddress,cfg.ExternalSignUrl, etc. While these aren't secrets, consider using a boolean indicator instead of printing potentially empty values.🔎 Proposed improvement
- return nil, fmt.Errorf("external sign is enabled but missing required config: address=%s, url=%s, appid=%s, chain=%s", - cfg.ExternalSignAddress, cfg.ExternalSignUrl, cfg.ExternalSignAppid, cfg.ExternalSignChain) + var missing []string + if cfg.ExternalSignAddress == "" { + missing = append(missing, "address") + } + if cfg.ExternalSignUrl == "" { + missing = append(missing, "url") + } + if cfg.ExternalSignAppid == "" { + missing = append(missing, "appid") + } + if cfg.ExternalSignChain == "" { + missing = append(missing, "chain") + } + if cfg.ExternalSignRsaPriv == "" { + missing = append(missing, "rsa-priv") + } + return nil, fmt.Errorf("external sign is enabled but missing required config: %v", missing)token-price-oracle/client/l2_client.go (1)
152-155: Consider returning a copy of chainID to prevent external mutation.
GetChainID()returns the internal*big.Intpointer directly. Whilebig.Intis typically used immutably, a caller could potentially mutate it. Consider returning a copy for defensive programming.🔎 Proposed fix
// GetChainID returns the chain ID func (c *L2Client) GetChainID() *big.Int { - return c.chainID + return new(big.Int).Set(c.chainID) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
token-price-oracle/client/l2_client.gotoken-price-oracle/client/sign.gotoken-price-oracle/cmd/main.gotoken-price-oracle/config/config.gotoken-price-oracle/flags/flags.gotoken-price-oracle/updater/tx_manager.go
🧰 Additional context used
🧬 Code graph analysis (4)
token-price-oracle/cmd/main.go (1)
token-price-oracle/client/l2_client.go (1)
NewL2Client(27-102)
token-price-oracle/config/config.go (1)
token-price-oracle/flags/flags.go (6)
ExternalSignFlag(144-148)ExternalSignAddressFlag(150-154)ExternalSignAppidFlag(156-160)ExternalSignChainFlag(162-166)ExternalSignUrlFlag(168-172)ExternalSignRsaPrivFlag(174-178)
token-price-oracle/client/l2_client.go (2)
token-price-oracle/client/sign.go (2)
Signer(17-24)NewSigner(27-60)token-price-oracle/config/config.go (1)
Config(53-88)
token-price-oracle/client/sign.go (1)
token-price-oracle/client/l2_client.go (1)
L2Client(18-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
🔇 Additional comments (11)
token-price-oracle/client/sign.go (2)
62-73: TheSignmethod only supports external signing, which may cause confusion.The method name
Signand struct nameSignersuggest general signing capability, but the implementation returns an error for local signing. This is by design per the comment on line 62, but callers might be confused. The error message correctly directs users to usebind.TransactOptsfor local signing.
111-119: Gas fee cap calculation follows standard EIP-1559 best practices.The calculation
tip + 2*baseFeeprovides headroom for base fee increases across blocks, which is the recommended approach for reliable transaction inclusion.token-price-oracle/flags/flags.go (1)
143-178: External sign flags are well-defined with consistent naming conventions.The new flags follow the existing patterns for naming (
external-sign-*) and environment variables (TOKEN_PRICE_ORACLE_EXTERNAL_SIGN_*).token-price-oracle/cmd/main.go (1)
107-111: Constructor signature change looks correct.The
NewL2Clientcall now passes the full config object, enabling the client to determine signing mode internally. This is a clean API change that encapsulates the signing configuration.token-price-oracle/config/config.go (1)
68-75: External sign configuration fields are properly defined.The struct fields align with the corresponding flags and provide all necessary configuration for external signing.
token-price-oracle/updater/tx_manager.go (2)
82-134: External signing flow is well-structured.The
sendWithExternalSignmethod correctly:
- Validates the signer is initialized
- Builds an unsigned transaction to extract calldata
- Delegates signing to the external signer
- Sends the signed transaction
- Waits for receipt with appropriate timeout
The flow is consistent with the local signing path.
35-39: Clean branching between local and external signing modes.The
SendTransactionmethod cleanly delegates to the appropriate signing implementation based on configuration.token-price-oracle/client/l2_client.go (4)
52-77: External signing initialization is well-implemented.The external signing path correctly:
- Parses the RSA private key
- Creates a new Signer with all required configuration
- Sets up TransactOpts with NoSend=true for building unsigned transactions
- Logs initialization details
78-99: Local signing initialization handles the0xprefix correctly.The code properly strips the optional
0xprefix from the private key hex string before parsing. The boundary checklen(cfg.PrivateKey) > 2prevents index out of bounds errors.
114-130:GetOptsreturns a defensive copy to prevent shared state issues.This is a good practice to avoid concurrent modification issues when multiple goroutines use the transaction options.
33-38: Deferred cleanup pattern is correctly implemented.The defer block correctly closes the client on failure. All error assignments within the function scope update the same
errvariable using:=, ensuring the cleanup checkif err != nilcatches errors from operations at lines 41, 54, 84, and 90. The early return at line 30 happens before the defer is established, so the pattern is safe.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
token-price-oracle/client/l2_client.go (1)
127-141: Shallow copy may not prevent concurrent modification issues.The
GetOpts()method performs a shallow copy, which means pointer fields (Nonce,Value,GasPrice,GasFeeCap,GasTipCap) still reference the same underlying*big.Intvalues. If callers mutate these values (e.g.,opts.Nonce.Add(...)), concurrent goroutines could experience race conditions.While typical usage often replaces these pointers rather than mutating them, the shallow copy doesn't fully "prevent shared state issues" as the comment claims.
🔎 Options to address this issue
Option 1: Deep copy big.Int fields (recommended if concurrent access is expected)
func (c *L2Client) GetOpts() *bind.TransactOpts { - // Return a copy to prevent shared state issues - return &bind.TransactOpts{ + opts := &bind.TransactOpts{ From: c.opts.From, - Nonce: c.opts.Nonce, Signer: c.opts.Signer, - Value: c.opts.Value, - GasPrice: c.opts.GasPrice, - GasFeeCap: c.opts.GasFeeCap, - GasTipCap: c.opts.GasTipCap, GasLimit: c.opts.GasLimit, Context: c.opts.Context, NoSend: c.opts.NoSend, } + + // Deep copy big.Int pointers + if c.opts.Nonce != nil { + opts.Nonce = new(big.Int).Set(c.opts.Nonce) + } + if c.opts.Value != nil { + opts.Value = new(big.Int).Set(c.opts.Value) + } + if c.opts.GasPrice != nil { + opts.GasPrice = new(big.Int).Set(c.opts.GasPrice) + } + if c.opts.GasFeeCap != nil { + opts.GasFeeCap = new(big.Int).Set(c.opts.GasFeeCap) + } + if c.opts.GasTipCap != nil { + opts.GasTipCap = new(big.Int).Set(c.opts.GasTipCap) + } + + return opts }Option 2: Update the comment to reflect actual behavior
- // Return a copy to prevent shared state issues + // Return a shallow copy of transaction options + // Note: Pointer fields (Nonce, Value, GasPrice, etc.) reference the same underlying values + // Callers should not mutate these fields directly return &bind.TransactOpts{Option 3: Add synchronization if concurrent access is required
Consider using a
sync.RWMutexto protect access toc.optsif concurrent reads/writes are expected.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
token-price-oracle/client/l2_client.go
🧰 Additional context used
🧬 Code graph analysis (1)
token-price-oracle/client/l2_client.go (2)
token-price-oracle/client/sign.go (2)
Signer(17-24)NewSigner(27-60)token-price-oracle/config/config.go (1)
Config(53-88)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
token-price-oracle/client/l2_client.go (4)
19-25: LGTM: Struct changes support dual signing modes.The addition of
signerandexternalSignfields appropriately supports both external and local signing modes.
89-110: LGTM: Local signing initialization is solid.The local signing path properly handles:
- Defensive stripping of "0x" prefix
- Appropriate error handling and wrapping
- Standard go-ethereum key derivation
- Clear logging
153-166: LGTM: Accessor methods are clean and well-documented.The new accessor methods appropriately expose the signing mode, signer, and chain ID for downstream usage.
75-77: NoSend flag and external signing flow are properly implemented.Verification confirms the code correctly handles all three concerns:
- NoSend flag respected: Set to
true(lines 49, 92) for transaction building/gas estimation and never allows direct sending in the external signing path- Transaction re-signed before broadcast: Line 110 explicitly calls
signer.CreateAndSignTx()to create and sign the transaction- No unsafe code paths: Only the
signedTxresult from the external signer (line 110) is broadcast viaSendTransaction(line 116); the original unsigned transaction is never sentThe implementation safely separates unsigned transaction building with
NoSend=truefrom the external signing and broadcast steps.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
token-price-oracle/go.mod (1)
3-3: Ensure team compatibility with Go 1.24.0 and review breaking changes.Go 1.24.0 was officially released February 11, 2025, and is a stable version under the Go 1 compatibility promise. However, verify that:
- All developers and CI/CD environments use this version consistently
- Your module's code is tested against 1.24.0, as minor breaking changes exist:
- Generic type aliases behavior has changed (flag will be removed in 1.25)
- cgo-generated type receiver restrictions are now enforced
- go vet analyzers may surface new diagnostics
- testing.AllocsPerRun now panics if parallel tests are running
- You have reviewed the Go 1.24 release notes if this version is above your minimum requirement
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
token-price-oracle/go.sumis excluded by!**/*.sum
📒 Files selected for processing (1)
token-price-oracle/go.mod
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (rust)
🔇 Additional comments (1)
token-price-oracle/go.mod (1)
39-39: v2.13.1 is a legitimate version with no known security vulnerabilities.The dependency on
github.com/go-resty/resty/v2 v2.13.1is safe to use. Earlier CVE-2023-45286 (race condition causing request-body disclosure) affected versions up to v2.10.0 and was fixed in v2.11.0, so v2.13.1 is not affected. No other public CVE entries exist for this version.Note: v2.13.1 is several minor versions behind the latest release (v2.17.1 as of Dec 15, 2025). Consider updating to the latest version during the next dependency maintenance cycle if no compatibility constraints prevent it.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
token-price-oracle/updater/tx_manager.go (1)
101-106: Nil check fortx.To()properly addresses past review feedback.The contract creation guard is now in place, preventing potential nil pointer dereference.
🧹 Nitpick comments (1)
token-price-oracle/config/config.go (1)
246-249: Weak RSA private key validation may cause confusing downstream errors.The current check only verifies that the string contains
"RSA PRIVATE KEY", which doesn't confirm the key is actually parseable. Consider parsing the key at config time to fail fast with a clear error message.🔎 Proposed enhancement using crypto/x509
+import ( + "crypto/x509" + "encoding/pem" +)// Validate RSA private key format - if !strings.Contains(cfg.ExternalSignRsaPriv, "RSA PRIVATE KEY") { - return nil, fmt.Errorf("invalid RSA private key format (must contain 'RSA PRIVATE KEY')") + block, _ := pem.Decode([]byte(cfg.ExternalSignRsaPriv)) + if block == nil || !strings.Contains(block.Type, "RSA PRIVATE KEY") { + return nil, fmt.Errorf("invalid RSA private key format: failed to decode PEM block") + } + if _, err := x509.ParsePKCS1PrivateKey(block.Bytes); err != nil { + // Try PKCS8 format as fallback + if _, err := x509.ParsePKCS8PrivateKey(block.Bytes); err != nil { + return nil, fmt.Errorf("invalid RSA private key: failed to parse key: %w", err) + } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
token-price-oracle/config/config.gotoken-price-oracle/updater/tx_manager.go
🧰 Additional context used
🧬 Code graph analysis (2)
token-price-oracle/updater/tx_manager.go (2)
bindings/hardhat/types.go (1)
Receipt(32-46)ops/l2-genesis/eth/types.go (1)
Data(123-123)
token-price-oracle/config/config.go (1)
token-price-oracle/flags/flags.go (6)
ExternalSignFlag(144-148)ExternalSignAddressFlag(150-154)ExternalSignAppidFlag(156-160)ExternalSignChainFlag(162-166)ExternalSignUrlFlag(168-172)ExternalSignRsaPrivFlag(174-178)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
token-price-oracle/config/config.go (3)
68-75: LGTM on the new Config fields.The fields are appropriately typed and follow the naming conventions consistent with the corresponding flags in
flags/flags.go.
96-103: Config loading looks correct.The external sign fields are loaded from their corresponding flags following the established pattern.
227-255: Good validation structure for the signing mode mutual exclusivity.The validation correctly enforces that either all external sign fields are provided, or a private key is required. The address and URL format checks are appropriate.
token-price-oracle/updater/tx_manager.go (4)
31-39: Clean routing pattern for signing mode selection.The mutex lock ensures thread-safety for both signing paths, and the routing logic is straightforward.
41-80: LGTM on the local signing flow.The refactored method preserves the original gas estimation (1.5x multiplier) and transaction sending logic with appropriate logging.
128-136: Consistent receipt waiting behavior across both signing paths.Both local and external signing paths use the same timeout (60s) and poll interval (2s), ensuring predictable behavior regardless of signing mode.
82-137: The external signing path already applies gas estimation logic comparable to the local path. Both the local signing flow (lines 55-58) andCreateAndSignTxin sign.go estimate gas and apply a 1.5x multiplier (50% buffer):gas * 3 / 2. No action required.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
token-price-oracle/config/config.go (2)
68-74: Consider following Go naming conventions for acronyms.The fields
ExternalSignUrlandExternalSignRsaPrivshould use all-caps for acronyms per Go conventions:ExternalSignURLandExternalSignRSAPriv.🔎 Proposed fix
// External sign ExternalSign bool ExternalSignAddress string ExternalSignAppid string ExternalSignChain string - ExternalSignUrl string - ExternalSignRsaPriv string + ExternalSignURL string + ExternalSignRSAPriv stringNote: This change would also require updates to the corresponding flag names in
LoadConfigand validation logic.
228-239: Consider adding format validation for URL and RSA private key.While the non-empty checks and hex address validation are good, additional validation would help catch configuration errors earlier:
- Parse and validate
ExternalSignUrlusingurl.Parseto ensure it's a valid URL- Validate
ExternalSignRsaPrivformat (e.g., PEM format validation)🔎 Example validation additions
Add this import at the top:
import ( "fmt" + "net/url" "morph-l2/bindings/predeploys"Then enhance the validation:
// Validate external sign config if cfg.ExternalSign { if cfg.ExternalSignAddress == "" || cfg.ExternalSignUrl == "" || cfg.ExternalSignAppid == "" || cfg.ExternalSignChain == "" || cfg.ExternalSignRsaPriv == "" { return nil, fmt.Errorf("external sign is enabled but missing required config: address=%s, url=%s, appid=%s, chain=%s", cfg.ExternalSignAddress, cfg.ExternalSignUrl, cfg.ExternalSignAppid, cfg.ExternalSignChain) } // Validate address format if !common.IsHexAddress(cfg.ExternalSignAddress) { return nil, fmt.Errorf("invalid external sign address format: %s", cfg.ExternalSignAddress) } + + // Validate URL format + if _, err := url.Parse(cfg.ExternalSignUrl); err != nil { + return nil, fmt.Errorf("invalid external sign URL format: %s: %w", cfg.ExternalSignUrl, err) + } } else {Note: RSA key validation would require additional crypto libraries and depends on the expected format.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
token-price-oracle/config/config.go
🧰 Additional context used
🧬 Code graph analysis (1)
token-price-oracle/config/config.go (1)
token-price-oracle/flags/flags.go (6)
ExternalSignFlag(144-148)ExternalSignAddressFlag(150-154)ExternalSignAppidFlag(156-160)ExternalSignChainFlag(162-166)ExternalSignUrlFlag(168-172)ExternalSignRsaPrivFlag(174-178)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (rust)
🔇 Additional comments (1)
token-price-oracle/config/config.go (1)
96-102: LGTM!Configuration loading is straightforward and correctly retrieves all external signing parameters from CLI flags.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
token-price-oracle/updater/tx_manager.go (1)
154-156: Consider logging nonce for consistency with local sign path.The local sign log (line 97) includes
noncewhich is useful for debugging transaction ordering issues. The external sign log omits this. IfsignedTx.Nonce()is available, consider adding it for observability parity.🔎 Proposed fix
log.Info("Transaction sent (external sign)", "tx_hash", signedTx.Hash().Hex(), + "nonce", signedTx.Nonce(), "gas_limit", signedTx.Gas())
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
token-price-oracle/updater/tx_manager.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (rust)
🔇 Additional comments (2)
token-price-oracle/updater/tx_manager.go (2)
36-40: Clean routing logic for signing paths.The branching based on
IsExternalSign()is straightforward and maintains clear separation between the two signing flows.
42-110: LGTM!The local signing flow is well-structured with appropriate pending transaction checks, gas estimation with 1.5x buffer, and clear logging.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
token-price-oracle/updater/tx_manager.go (2)
44-69: Consider extracting duplicate pending transaction check logic.The pending transaction check logic (lines 46-69 and 121-144) is nearly identical in both
sendWithLocalSignandsendWithExternalSign. This duplication increases maintenance burden and risk of divergence.🔎 Proposed refactor
Extract a helper method:
+// checkPendingTransactions returns an error if there are pending transactions +func (m *TxManager) checkPendingTransactions(ctx context.Context, fromAddr common.Address) error { + confirmedNonce, err := m.l2Client.GetClient().NonceAt(ctx, fromAddr, nil) + if err != nil { + return fmt.Errorf("failed to get confirmed nonce: %w", err) + } + + pendingNonce, err := m.l2Client.GetClient().PendingNonceAt(ctx, fromAddr) + if err != nil { + return fmt.Errorf("failed to get pending nonce: %w", err) + } + + if pendingNonce > confirmedNonce { + log.Warn("Found pending transactions, skipping this round", + "address", fromAddr.Hex(), + "confirmed_nonce", confirmedNonce, + "pending_nonce", pendingNonce, + "pending_count", pendingNonce-confirmedNonce) + return fmt.Errorf("pending transactions exist (confirmed: %d, pending: %d)", confirmedNonce, pendingNonce) + } + + log.Info("No pending transactions, proceeding to send", + "address", fromAddr.Hex(), + "nonce", confirmedNonce) + return nil +}Then use it in both methods:
if err := m.checkPendingTransactions(ctx, fromAddr); err != nil { return nil, err }Also applies to: 119-144
181-183: Consider logging the nonce for external sign transactions.The local sign path logs the nonce (line 97), but the external sign path omits it. Including the nonce would improve debugging consistency.
🔎 Proposed fix
log.Info("Transaction sent (external sign)", "tx_hash", signedTx.Hash().Hex(), + "nonce", signedTx.Nonce(), "gas_limit", signedTx.Gas())token-price-oracle/client/l2_client.go (2)
94-97: Consider usingstrings.TrimPrefixfor cleaner 0x stripping.The manual prefix check works but could be more idiomatic.
🔎 Proposed fix
+import "strings" + } else { // Local private key mode - privateKeyHex := cfg.PrivateKey - if len(cfg.PrivateKey) > 2 && cfg.PrivateKey[:2] == "0x" { - privateKeyHex = cfg.PrivateKey[2:] - } + privateKeyHex := strings.TrimPrefix(cfg.PrivateKey, "0x") key, err := crypto.HexToECDSA(privateKeyHex)Note:
stringsis already commonly imported elsewhere, andTrimPrefixhandles the case where the prefix doesn't exist gracefully.
166-168: Consider returning a copy of chainID to prevent external mutation.
GetChainID()returns the internal*big.Intdirectly. Sincebig.Intis mutable, external callers could inadvertently modify the client's chain ID.🔎 Proposed fix
// GetChainID returns the chain ID func (c *L2Client) GetChainID() *big.Int { - return c.chainID + return new(big.Int).Set(c.chainID) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
token-price-oracle/client/l2_client.gotoken-price-oracle/config/config.gotoken-price-oracle/updater/tx_manager.go
🧰 Additional context used
🧬 Code graph analysis (3)
token-price-oracle/client/l2_client.go (1)
token-price-oracle/client/sign.go (2)
Signer(17-24)NewSigner(27-60)
token-price-oracle/updater/tx_manager.go (2)
bindings/hardhat/types.go (1)
Receipt(32-46)ops/l2-genesis/eth/types.go (1)
Data(123-123)
token-price-oracle/config/config.go (1)
token-price-oracle/flags/flags.go (6)
ExternalSignFlag(144-148)ExternalSignAddressFlag(150-154)ExternalSignAppidFlag(156-160)ExternalSignChainFlag(162-166)ExternalSignUrlFlag(168-172)ExternalSignRsaPrivFlag(174-178)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
token-price-oracle/config/config.go (2)
68-74: LGTM - External signing configuration fields are well-structured.The new configuration fields cover all necessary parameters for external signing: enable flag, address, app ID, chain identifier, URL, and RSA private key.
227-245: Validation logic is comprehensive and secure.Good improvements:
- All five required fields are checked when
ExternalSignis enabled- Address format validation using
common.IsHexAddress- RSA private key presence is logged as boolean (
rsa_priv_set=%t) rather than exposing the actual key value - this is a security-conscious approach- Mutual exclusivity enforced: private key required when external sign is disabled
token-price-oracle/updater/tx_manager.go (2)
36-40: Clean routing between signing modes.The branching logic clearly separates the two signing paths while maintaining the mutex lock for thread safety.
159-161: Good addition of nil check for contract creation transactions.This addresses the potential nil pointer dereference that was flagged in previous reviews.
token-price-oracle/client/l2_client.go (3)
73-87: Well-documented placeholder signer with clear safety constraints.The extensive comments (lines 73-78) and the
CRITICALnote on line 81 clearly document why the placeholder signer exists and its safety guarantees. TheNoSend: truesetting prevents accidental broadcast, andtx_manager.goonly extractsTo()andData()from the placeholder transaction before creating a properly signed one.This addresses the previous review concern about the placeholder signer.
53-68: External signing initialization relies on config-level validation.The code assumes
cfg.ExternalSignRsaPrivand other fields are valid because validation now happens inconfig/config.go(lines 227-245). This is a clean separation of concerns.
156-168: LGTM - Clean accessor methods.The new accessor methods (
IsExternalSign,GetSigner,GetChainID) provide proper encapsulation and allowtx_manager.goto query the signing mode and access the signer as needed.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
token-price-oracle/README.md (2)
92-94: Add language identifier to fenced code block.The formula block should specify a language identifier for consistency with Markdown best practices.
🔎 Proposed fix
- ``` + ```text priceRatio = tokenScale × tokenPriceUSD × 10^(18 - tokenDecimals) / ethPriceUSD - ``` + ```
145-161: Add language identifier to project structure tree block.The directory tree block should specify a language identifier for consistency with Markdown best practices.
🔎 Proposed fix
- ``` + ```text token-price-oracle/ ├── cmd/ # Entry point ├── flags/ # CLI flags definition ├── config/ # Configuration loading ├── client/ # Client wrappers │ ├── l2_client.go # L2 chain client │ ├── price_feed.go # Price feed interface │ ├── bitget_sdk.go # Bitget API client │ └── sign.go # External signing ├── updater/ # Update logic │ ├── token_price.go # Price updater │ ├── tx_manager.go # Transaction manager │ └── factory.go # Factory methods ├── metrics/ # Prometheus metrics └── README.md # This document - ``` + ```
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
token-price-oracle/README.md
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
token-price-oracle/README.md
92-92: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
145-145: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (rust)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
token-price-oracle/README.md (1)
15-53: Add Quick Start example for external signing mode.The Quick Start section (lines 15–53) demonstrates only local signing mode. Since the documentation recommends external signing for production (line 84), users should see a complete example for external signing as well. This would help operators quickly adopt the recommended approach.
Consider adding a collapsible or separate subsection showing environment variables for external signing mode, similar to the local signing example at lines 17–32.
🔎 Proposed addition
> **Note**: `PRIVATE_KEY` is only required when using local signing mode. For production, use [External Signing](#external-signing-recommended-for-production) instead. +### Environment Variables (External Signing Mode) + +```bash +# Required +export TOKEN_PRICE_ORACLE_L2_ETH_RPC="https://rpc.morphl2.io" +export TOKEN_PRICE_ORACLE_EXTERNAL_SIGN="true" +export TOKEN_PRICE_ORACLE_EXTERNAL_SIGN_ADDRESS="0x..." +export TOKEN_PRICE_ORACLE_EXTERNAL_SIGN_APPID="your-app-id" +export TOKEN_PRICE_ORACLE_EXTERNAL_SIGN_URL="https://external-sign-service.example.com" +export TOKEN_PRICE_ORACLE_EXTERNAL_SIGN_RSA_PRIV="-----BEGIN RSA PRIVATE KEY-----..." +export TOKEN_PRICE_ORACLE_BITGET_API_BASE_URL="https://api.bitget.com" +export TOKEN_PRICE_ORACLE_TOKEN_MAPPING_BITGET="1:BTCUSDT,2:ETHUSDT" + +# Optional +export TOKEN_PRICE_ORACLE_EXTERNAL_SIGN_CHAIN="morphl2" +export TOKEN_PRICE_ORACLE_PRICE_UPDATE_INTERVAL="1m" +export TOKEN_PRICE_ORACLE_PRICE_THRESHOLD="100" # 1% (100 bps) +export TOKEN_PRICE_ORACLE_METRICS_SERVER_ENABLE="true" +export TOKEN_PRICE_ORACLE_METRICS_PORT="6060" +export TOKEN_PRICE_ORACLE_LOG_LEVEL="info" +``` + ### Build and Run
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
token-price-oracle/README.md
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
token-price-oracle/README.md
99-99: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
152-152: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
| Uses: ../bindings/bindings (project root contract bindings) | ||
| ### Price Ratio Formula | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Specify language for fenced code block.
The formula block is missing a language specifier. Since it's mathematical notation, use plain text formatting or add a language hint.
🔎 Proposed fix
-```
+```text
priceRatio = tokenScale × tokenPriceUSD × 10^(18 - tokenDecimals) / ethPriceUSD
-```
+```📝 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.
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
99-99: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In token-price-oracle/README.md around line 99 the fenced code block with the
formula lacks a language specifier; update the closing and opening fence to
include a hint such as ```text (or ```plain) and ensure the formula is wrapped
between matching fences so it renders as a plain-text code block: replace the
current triple-backtick fences with fenced code block markers that include the
chosen language tag and keep the formula line unchanged.
| ## Project Structure | ||
| ## Testing | ||
| ``` |
There was a problem hiding this comment.
Specify language for fenced code block.
The directory tree is missing a language specifier. Use text or leave it plain for tree structures.
🔎 Proposed fix
-```
+```text
token-price-oracle/
├── cmd/ # Entry point
├── flags/ # CLI flags definition
@@ ... @@
└── README.md # This document
-```
+```📝 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.
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
152-152: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In token-price-oracle/README.md around line 152, the fenced code block for the
directory tree lacks a language specifier; update the opening fence to specify a
language such as "text" (i.e. change ``` to ```text) and ensure the closing
fence remains ``` so the tree is rendered correctly as plain text.
Summary by CodeRabbit
New Features
Configuration
Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.