feat: add EIP-7702 aggregator-sponsored transactions as fallback#708
Open
onahprosper wants to merge 19 commits intomainfrom
Open
feat: add EIP-7702 aggregator-sponsored transactions as fallback#708onahprosper wants to merge 19 commits intomainfrom
onahprosper wants to merge 19 commits intomainfrom
Conversation
…ions Migrate EVM receive addresses from Thirdweb's EIP-4337 smart wallet infrastructure to native EIP-7702 delegation. This removes third-party dependencies, centralizes gas sponsorship through a single keeper wallet (AggregatorEVMPrivateKey), and uses direct RPC for all on-chain operations. Key changes: - Add EIP-7702 helpers (delegation check, authorization signing, batch signing, keeper transaction submission) in utils/eip7702.go - Add per-(chainID, address) NonceManager for concurrent keeper transactions - Rewrite CreateOrder, SettleOrder, RefundOrder to use keeper-sponsored EIP-7702 transactions instead of Thirdweb Engine - Replace Thirdweb event polling with direct RPC-based event fetching (services/indexer/rpc_events.go) - Remove EngineService entirely (services/engine.go) - Remove Thirdweb webhook creation from main.go and sender controller - Generate EOA keypairs for receive addresses instead of smart wallets - Add delegation_contract_address field to Network entity schema - Add AggregatorEVMPrivateKey config for on-chain EVM signing
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds EIP-7702 keeper-sponsored flows and per-chain nonce management: receive addresses can be fresh EOAs with encrypted salts; CreateOrder/Settle/Refund use keeper RPC submissions or Thirdweb Engine depending on sponsorship_mode; network schema gains delegation_contract_address and sponsorship_mode; payment orders gain wallet_type; webhook and engine wiring adjusted. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Sender as SenderController
participant RAS as ReceiveAddressService
participant Engine as EngineService
participant OrderEVM as OrderEVM
participant Nonce as NonceManager
participant EIP7702 as EIP-7702 Utils
participant RPC as Ethereum RPC
participant Keeper as KeeperWallet
Client->>Sender: InitiatePaymentOrder
Sender->>RAS: CreateSmartAddress(ctx,label,mode)
alt mode == self_sponsored
RAS->>RAS: Generate EOA, Encrypt private key -> salt
RAS-->>Sender: address, salt
else mode == thirdweb
RAS->>Engine: CreateServerWallet(label)
Engine-->>RAS: address
RAS-->>Sender: address, nil
end
Sender->>OrderEVM: CreateOrder(order with walletType, salt)
alt walletType == eoa_7702
OrderEVM->>EIP7702: Decrypt salt -> userKey
OrderEVM->>EIP7702: SignBatch7702(userKey, calls)
OrderEVM->>Nonce: AcquireNonce(chainID, keeperAddr)
OrderEVM->>EIP7702: SendKeeperTx(keeperKey, calldata, auths)
EIP7702->>RPC: eth_sendRawTransaction / poll receipt
RPC-->>EIP7702: receipt
else walletType == smart_wallet
OrderEVM->>Engine: SendTransactionBatch(...)
Engine-->>OrderEVM: result
end
OrderEVM->>DB: Update PaymentOrder (txHash/status)
OrderEVM-->>Client: Respond
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/indexer/evm.go (1)
337-338:⚠️ Potential issue | 🟡 MinorStale "falling back to Engine" log messages and lost error context for Lisk.
Two related issues in
getAddressTransactionHistoryWithFallbackAndBypass:
Stale log messages (lines 337, 348): Both
Warnfcalls still say"falling back to Engine", but the Engine service was removed. Operators will be misled by these messages.
errmay benilat line 351 for chain 1135: The blockscout block at line 341 uses a short declaration (:=), so the innererris scoped to thatifblock. When blockscout fails, the outererr(declared at line 322) is never updated — it staysnil. The%wwrap on line 351 therefore produces"transaction history not available for chain 1135 via etherscan or blockscout: <nil>", discarding the actual blockscout failure reason.🐛 Proposed fix
- logger.Warnf("Etherscan failed for chain %d, falling back to Engine: %v", chainID, err) + logger.Warnf("Etherscan failed for chain %d, falling back to Blockscout: %v", chainID, err)if chainID == 1135 { - transactions, err := s.blockscoutService.GetAddressTokenTransfers(ctx, chainID, address, limit, fromBlock, toBlock) + var transactions []map[string]interface{} + transactions, err = s.blockscoutService.GetAddressTokenTransfers(ctx, chainID, address, limit, fromBlock, toBlock) if err == nil { return transactions, nil } - logger.Warnf("Blockscout failed for chain %d, falling back to Engine: %v", chainID, err) + logger.Warnf("Blockscout failed for chain %d: %v", chainID, err) }Also applies to: 348-351
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/indexer/evm.go` around lines 337 - 338, Update getAddressTransactionHistoryWithFallbackAndBypass to (1) change both logger.Warnf calls so they no longer mention "falling back to Engine" — instead say something like "falling back to alternate provider" or name the actual fallback (e.g., "blockscout"), and include the current error details; and (2) fix the err scoping bug by replacing the inner short-declaration (:=) used when calling blockscout with an assignment to the outer err (e.g., err = ...) so the outer err reflects blockscout failures, then use that non-nil err when wrapping or logging (fmt.Errorf("transaction history not available for chain %d via etherscan or blockscout: %w", chainID, err)). Ensure both places that previously used the stale message and the wrapped error are updated to reference the corrected err and the new log text.
🧹 Nitpick comments (10)
controllers/sender/sender.go (1)
491-501: Pre-existing:Save(ctx)should beSave(reqCtx)for the transaction log.Line 495 passes the Gin context
ctxto.Save()instead of the request contextreqCtx(defined at line 101). The payment order on line 531 correctly usesreqCtx. This is inconsistent and violates the coding guidelines for this handler.Suggested fix
- Save(ctx) + Save(reqCtx)As per coding guidelines, "In Gin HTTP handlers, do not pass
ctx(*gin.Context) to storage, Redis, or database calls. Extract request context withreqCtx := ctx.Request.Context()at the start of each handler and usereqCtxfor all storage.Client, db.Client, and storage.RedisClient operations".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controllers/sender/sender.go` around lines 491 - 501, The transaction log creation is calling Save(ctx) with the Gin context; change it to use the extracted request context variable reqCtx instead (i.e., replace the Save(ctx) call in the tx.TransactionLog.Create().SetStatus(...).SetNetwork(...).Save(...) chain with Save(reqCtx)); ensure reqCtx (from ctx.Request.Context()) is used consistently for storage/db calls in this handler just like the later payment order code.services/common/order.go (1)
846-849: Dead call site and stale comments remain after convertingdeleteTransferWebhookto a no-op.Since
deleteTransferWebhookalways returnsnil, the call site inprocessPaymentOrderPostCreation(lines 957–962) — including the error guard andlogger.Errorf— is now unreachable dead code. The surrounding comments also became stale:
- Line 947 (
processPaymentOrderPostCreationdoc): still says "webhook deletion".- Line 957 inline comment: "Delete the transfer webhook now that payment order is created/updated".
Consider removing both the dead function and its call site, or at a minimum stripping the stale comments:
♻️ Proposed clean-up
-// deleteTransferWebhook is a no-op now -func deleteTransferWebhook(_ context.Context, _ string) error { - return nil -}-// processPaymentOrderPostCreation handles post-creation tasks: webhook deletion, webhook sending, AML check, and provider assignment. +// processPaymentOrderPostCreation handles post-creation tasks: webhook sending, AML check, and provider assignment. func processPaymentOrderPostCreation(- // Delete the transfer webhook now that payment order is created/updated - err := deleteTransferWebhook(ctx, event.TxHash) - if err != nil { - logger.Errorf("Failed to delete transfer webhook for payment order: %v", err) - // Don't fail the entire operation if webhook deletion fails - } - // Send webhook notification to sender err = utils.SendPaymentOrderWebhook(ctx, paymentOrder)As per coding guidelines, changes to the order lifecycle in
services/common/order.goshould be discussed before merging: "Ask before changing refund/fulfillment flows intasks/stale_ops.goandtasks/refunds.go, or order lifecycle inservices/common/order.go".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/common/order.go` around lines 846 - 849, The deleteTransferWebhook function is now a no-op returning nil, so remove its dead call and associated error handling/comment in processPaymentOrderPostCreation (references: deleteTransferWebhook and processPaymentOrderPostCreation) and update the doc comment on processPaymentOrderPostCreation that still mentions "webhook deletion"; either delete both the function and the call site or remove the call plus stale inline/doc comments so no unreachable error-guard remains. Before removing or changing lifecycle behavior in services/common/order.go, raise the change for discussion per the repo guideline about altering order lifecycle/refund flows.utils/nonce_manager.go (2)
104-111: String-matching for nonce errors is fragile.Different Ethereum clients (Geth, Erigon, Nethermind, etc.) use varying error messages. This list may need to expand over time. Consider adding a comment documenting which clients these strings correspond to, and keep this in mind when onboarding new RPC providers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/nonce_manager.go` around lines 104 - 111, The isNonceTooLow function currently relies on fragile string-matching against error messages; update it by adding a clear comment above isNonceTooLow that lists which clients each matched substring corresponds to (e.g., Geth, Erigon, Nethermind, OpenEthereum) and note that this list must be expanded when onboarding new RPC providers, and then factor the literals into named constants or a slice (e.g., nonceErrSignatures) so future additions are obvious and centralized; ensure the comment also advises preferencing structured error codes if/when providers expose them.
31-48:AcquireNonceholds the mutex during the RPC callPendingNonceAt.On the first call for a given key (cache miss), the mutex is held for the entire duration of the
PendingNonceAtRPC round-trip (lines 38-42). This blocks every other goroutine callingAcquireNoncefor any chain/address. Under concurrent load across multiple chains, a slow RPC endpoint would stall all nonce acquisitions.Consider doing the RPC call outside the lock (double-check pattern) or using per-key locking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/nonce_manager.go` around lines 31 - 48, AcquireNonce currently holds nm.mu while calling client.PendingNonceAt which blocks all AcquireNonce callers; change to fetch pendingNonce outside the global lock using a double-check or per-key lock: first lock nm.mu and check nm.nonces[key], if missing release the lock, call client.PendingNonceAt(ctx, addr) to get pendingNonce, then re-lock nm.mu and check nm.nonces[key] again (if still missing set nm.nonces[key]=pendingNonce), then increment and return the nonce; reference functions/types: AcquireNonce, NonceManager.mu, nonceKey, nm.nonces, and client.PendingNonceAt.tasks/refunds.go (1)
186-193: ABI parsing is repeated per order inside each goroutine.
abi.JSON(strings.NewReader(contracts.ERC20TokenMetaData.ABI))is deterministic and could be parsed once before the loop, avoiding repeated work.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tasks/refunds.go` around lines 186 - 193, The ERC20 ABI is being parsed inside each goroutine using abi.JSON(strings.NewReader(contracts.ERC20TokenMetaData.ABI)), causing redundant work; parse contracts.ERC20TokenMetaData.ABI once before spawning the per-order goroutines (e.g., create a single erc20ABI via abi.JSON and handle its error upfront) and then reuse that erc20ABI inside the loop/goroutines (referencing the erc20ABI symbol) so each goroutine does not reparse the ABI.utils/nonce_manager_test.go (1)
234-275:TestSubmitWithNonce_NonceTooLowTriggersResetdoesn't exercise the retry loop.This test manually calls
AcquireNonce,isNonceTooLow, andResetNoncein sequence rather than invokingSubmitWithNoncewith a function that returns a nonce-too-low error. It doesn't actually validate thatSubmitWithNonceresets and retries. Consider adding a test that passes asubmitFnwhich fails with "nonce too low" on the first call and succeeds on the second (after re-seeding the nonce in between via a goroutine or callback counter).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/nonce_manager_test.go` around lines 234 - 275, Update the test to exercise the SubmitWithNonce retry loop by invoking SubmitWithNonce (not just AcquireNonce/isNonceTooLow/ResetNonce) with a submitFn that fails with a nonce-too-low error on the first invocation and succeeds on the second; implement this by using a counter or goroutine to reseed the NonceManager.nonces (or call ResetNonce and then set nm.nonces[nonceKey{chainID, addr}] = <newNonce>) between attempts so SubmitWithNonce triggers a reset and retry path, and assert that SubmitWithNonce ultimately returns success and used the re-seeded nonce. Ensure you reference SubmitWithNonce, submitFn, isNonceTooLow, ResetNonce, and AcquireNonce in the test to locate and modify the behavior.utils/eip7702.go (1)
118-128: Hardcoded gas limit of 500,000 may be insufficient or wasteful.A static
gasLimit = 500_000doesn't account for the actual complexity of the transaction. For a simple refund, this wastes gas (overpaying); for a complex batch with large calldata, it could be insufficient. Consider usingclient.EstimateGas()with a safety margin:estimated, err := client.EstimateGas(ctx, ethereum.CallMsg{From: keeperAddr, To: &to, Data: data}) if err != nil { return nil, fmt.Errorf("gas estimation failed: %w", err) } gasLimit := estimated * 120 / 100 // 20% buffer🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/eip7702.go` around lines 118 - 128, Replace the hardcoded gasLimit in SendKeeperTx with a dynamic estimate: call client.EstimateGas using an ethereum.CallMsg built from the keeper address (derive keeperAddr from keeperKey), To: &to and Data: data, handle estimation errors, then set gasLimit to the estimated value plus a small safety margin (e.g., 20%), and still ensure gasLimit fits uint64; keep the existing fallback behavior only if estimation fails.services/order/evm.go (1)
272-272: Track the TODO items for production readiness.Lines 272 and 343 have TODOs about calling
IndexGatewaywith the tx hash instead of setting it directly. These should be tracked as follow-up tasks before this ships to production.Do you want me to open issues to track these?
Also applies to: 343-343
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/order/evm.go` at line 272, There are TODO notes left about setting the transaction hash directly instead of invoking IndexGateway; create tracked follow-up issues to replace the direct assignment with a call to IndexGateway(txHash) and wire the call where the TODOs exist (ensure IndexGateway is invoked with the tx hash rather than writing it directly), add unit/integration tests asserting IndexGateway is called and the txHash flow works, update error handling and logging around the IndexGateway call, and remove the TODOs once implemented; include acceptance criteria in each issue (call made, retries/errors handled, tests passing).services/indexer/rpc_events.go (1)
93-108: Consider using a typed struct instead ofmap[string]interface{}.The event representation uses untyped maps, which makes downstream consumers fragile (type assertions needed everywhere, no compile-time safety). A concrete struct for RPC events would improve maintainability and reduce runtime assertion risks, especially since
ProcessRPCEventsBySignaturealready relies on specific key names and types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/indexer/rpc_events.go` around lines 93 - 108, Replace the untyped map event construction with a concrete struct (e.g., type RPCEvent struct { BlockNumber uint64; TransactionHash string; LogIndex uint; Address string; Topics []common.Hash; Data []byte; Decoded struct { IndexedParams map[string]interface{} `json:"indexed_params"`; NonIndexedParams map[string]interface{} `json:"non_indexed_params"` } }) and use a slice of that struct instead of events []interface{}; update the creation site (the loop over logs) to populate the struct fields (match names/types used by ProcessRPCEventsBySignature) and adjust any downstream consumers (or convert the slice to []interface{} only at the boundary) so ProcessRPCEventsBySignature receives strongly typed data instead of map[string]interface{}.services/indexer/evm.go (1)
308-320: Stale function doc comments — "engine or blockscout" fallback no longer exists.All three helper functions still advertise an Engine fallback in their doc comments (lines 308, 313, 319-320), which was removed in this PR. This makes
go docoutput and code navigation misleading.✏️ Proposed comment updates
-// getAddressTransactionHistoryWithFallback tries etherscan first and falls back to engine or blockscout +// getAddressTransactionHistoryWithFallback tries etherscan first and falls back to blockscout (Lisk only) func (s *IndexerEVM) getAddressTransactionHistoryWithFallback(...) ... -// getAddressTransactionHistoryImmediate tries etherscan immediately without queuing and falls back to engine or blockscout +// getAddressTransactionHistoryImmediate tries etherscan immediately without queuing and falls back to blockscout (Lisk only) func (s *IndexerEVM) getAddressTransactionHistoryImmediate(...) ... -// getAddressTransactionHistoryWithFallbackAndBypass tries etherscan first and falls back to engine or blockscout -// with option to bypass queue for immediate processing +// getAddressTransactionHistoryWithFallbackAndBypass tries etherscan first and falls back to blockscout (Lisk only), +// with option to bypass queue for immediate processing. func (s *IndexerEVM) getAddressTransactionHistoryWithFallbackAndBypass(...) ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/indexer/evm.go` around lines 308 - 320, Update the stale doc comments to remove any mention of the removed "engine" fallback and accurately describe current behavior: for getAddressTransactionHistoryWithFallback, say it tries Etherscan first and falls back to Blockscout; for getAddressTransactionHistoryImmediate, say it tries Etherscan immediately (bypassing queue) and falls back to Blockscout; and for getAddressTransactionHistoryWithFallbackAndBypass update the header to reflect Etherscan with optional bypass and Blockscout fallback. Use the function names getAddressTransactionHistoryWithFallback, getAddressTransactionHistoryImmediate, and getAddressTransactionHistoryWithFallbackAndBypass to locate and update the comments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/indexer/rpc_events.go`:
- Around line 54-60: The validation currently rejects fromBlock == 0 which
disallows queries starting at genesis; update the guard in the validation block
(the fromBlock/toBlock checks around variables fromBlock, toBlock and
eventSignatures/ utils.TransferEventSignature) to allow 0 and instead check for
invalid negative/sentinel values (e.g., fromBlock < 0 || toBlock < 0) or a
designated sentinel (e.g., -1) and also ensure toBlock >= fromBlock; keep the
existing range limits (100 for transfer events, 1000 for others) unchanged.
- Around line 17-21: The RPC client returned by types.NewEthClient is not being
closed, causing a resource leak; update the RPCClient interface in
types/types.go to include a Close() error method (signature Close() error) so
implementations can expose Close, then in GetContractEventsRPC defer
client.Close() immediately after successfully obtaining client from
types.NewEthClient to ensure the connection is cleaned up; reference RPCClient
(types/types.go), types.NewEthClient, and GetContractEventsRPC when making these
changes.
- Around line 62-73: The loop building filterQuery.Topics in rpc_events.go
currently skips empty topic strings, which shifts subsequent topics out of
position; modify the loop inside the function that constructs filterQuery (the
for _, topic := range topics block) to append a nil entry (wildcard) when topic
== "" instead of skipping it so that filterQuery.Topics preserves positional
semantics (use filterQuery.Topics = append(filterQuery.Topics, nil) for empty
topics and append the parsed common.HexToHash slice for non-empty topics).
In `@services/order/evm.go`:
- Around line 156-172: The batchNonce is incorrectly hardcoded to 0 before
signing (in the SignBatch7702 call), which breaks retries if a prior batch
partially succeeded; instead query the on-chain batch nonce for the sender
(e.g., call the gateway contract/read method that returns the current batch
nonce for userAddr) and use that value when calling utils.SignBatch7702 and
utils.PackExecute; update the code around batchNonce, SignBatch7702,
PackExecute, and any error messages to handle and propagate errors from the
on-chain nonce fetch so retries use the correct nonce.
- Around line 126-139: Remove the commented-out delegation-check block to clean
up the file, and stop hardcoding nonce=0 when calling
utils.SignAuthorization7702 in CreateOrder; instead query the account's current
nonce from chain (e.g., via an existing helper like utils.GetAccountNonce /
utils.GetNonce or by calling the client's PendingNonceAt/NonceAt for userAddr)
and pass that value to SignAuthorization7702, with a short defensive
fallback/log if the nonce lookup fails; reference symbols: CreateOrder,
utils.SignAuthorization7702, userKey, userAddr, delegationContract.
- Around line 181-195: The closure passed to nonceManager.SubmitWithNonce
captures the outer variable receipt, causing retries to mis-handle a reverted
on-chain tx and release a consumed nonce; fix by declaring receipt as a local
variable inside the SubmitWithNonce closure in CreateOrder (and the same pattern
in RefundOrder and SettleOrder), and when utils.SendKeeperTx returns a receipt
(even if receipt.Status == 0) return a non-retriable error/sentinel that
indicates the nonce was consumed (e.g., wrap the error with a "nonce consumed"
marker) so SubmitWithNonce does not treat it as a nonce-too-low and return the
actual revert error to the caller instead of releasing the nonce.
In `@tasks/refunds.go`:
- Around line 237-256: After a successful refund SendKeeperTx (i.e., when
utils.DefaultNonceManager.SubmitWithNonce returns no error), update the order
status so it isn't retried: set order.Status to the same state used by
services/order/evm.go (e.g., StatusRefunding or StatusRefunded) and persist that
change (call the same repository/update method used elsewhere or invoke
RefundOrder flow in services/order/evm.go) before returning; also log the status
update and any persistence error. Ensure you reference
DefaultNonceManager.SubmitWithNonce, utils.SendKeeperTx, and the order status
field (StatusRefunding/StatusRefunded) so the change mirrors existing
RefundOrder behavior.
In `@utils/eip7702.go`:
- Around line 168-175: The polling loop in utils/eip7702.go blocks for up to 60s
without honoring context cancellation; update the loop that calls
client.TransactionReceipt(ctx, signedTx.Hash()) to check ctx.Done() between
retries (or before sleeping) and return early with ctx.Err() if cancelled.
Replace the unconditional time.Sleep(2 * time.Second) with a select that waits
on either ctx.Done() (return nil, ctx.Err()) or a 2s timer before the next
iteration, and also consider checking ctx.Err() before the first
TransactionReceipt call; reference client.TransactionReceipt, signedTx, and ctx
to find and fix the code.
- Around line 100-108: The ReadBatchNonce function currently ignores errors from
abi.JSON(batchNonceABI) and parsed.Pack("nonce"), which can produce nil/invalid
data and confusing behavior; update ReadBatchNonce to check and return errors
from abi.JSON(batchNonceABI) and from parsed.Pack("nonce") (wrapping them with
context like "failed to parse ABI" / "failed to pack nonce call"), before
calling client.CallContract, and only proceed to CallContract when both parsed
and data are valid; reference symbols: ReadBatchNonce, batchNonceABI, abi.JSON,
parsed.Pack, client.CallContract.
- Around line 47-49: In SignAuthorization7702 replace the unsafe conversion
big.NewInt(int64(nonce)) with a uint64-safe big.Int creation by using
new(big.Int).SetUint64(nonce); update the RLP-encoded slice construction
accordingly so the nonce is represented as the big.Int from SetUint64 to avoid
overflow when nonce > math.MaxInt64.
- Around line 32-45: CheckDelegation currently compares raw code bytes between
eoa and delegationContract which fails for EIP-7702 because delegated EOAs hold
a 23-byte designation (0xef0100 || 20-byte target) not the target's runtime
bytecode; update CheckDelegation to call client.CodeAt for eoa, verify
len(code)==23 and code[0..3]=={0xef,0x01,0x00}, extract the embedded 20-byte
address from code[3:23], convert it to a common.Address and compare it to the
delegationContract address (return true on match), otherwise fall back to
current behavior or return false; preserve existing error handling for CodeAt
calls.
In `@utils/nonce_manager.go`:
- Around line 86-99: SubmitWithNonce currently calls ReleaseNonce whenever
submitFn returns an error, but that incorrectly rolls back an already-consumed
nonce when the error happened after mining; change submitFn's contract (used by
SendKeeperTx) to return an additional boolean (e.g., nonceConsumed) or a
sentinel to indicate whether the nonce was actually consumed, update
SubmitWithNonce to only call ReleaseNonce(chainID, addr, nonce) when
nonceConsumed==false (and still call ResetNonce(chainID, addr) on isNonceTooLow
errors), and update callers (e.g., SendKeeperTx) to set the consumed flag when
they observed a receipt; ensure AcquireNonce/ResetNonce semantics remain
unchanged so the in-memory nonce is not reverted for consumed nonces.
---
Outside diff comments:
In `@services/indexer/evm.go`:
- Around line 337-338: Update getAddressTransactionHistoryWithFallbackAndBypass
to (1) change both logger.Warnf calls so they no longer mention "falling back to
Engine" — instead say something like "falling back to alternate provider" or
name the actual fallback (e.g., "blockscout"), and include the current error
details; and (2) fix the err scoping bug by replacing the inner
short-declaration (:=) used when calling blockscout with an assignment to the
outer err (e.g., err = ...) so the outer err reflects blockscout failures, then
use that non-nil err when wrapping or logging (fmt.Errorf("transaction history
not available for chain %d via etherscan or blockscout: %w", chainID, err)).
Ensure both places that previously used the stale message and the wrapped error
are updated to reference the corrected err and the new log text.
---
Nitpick comments:
In `@controllers/sender/sender.go`:
- Around line 491-501: The transaction log creation is calling Save(ctx) with
the Gin context; change it to use the extracted request context variable reqCtx
instead (i.e., replace the Save(ctx) call in the
tx.TransactionLog.Create().SetStatus(...).SetNetwork(...).Save(...) chain with
Save(reqCtx)); ensure reqCtx (from ctx.Request.Context()) is used consistently
for storage/db calls in this handler just like the later payment order code.
In `@services/common/order.go`:
- Around line 846-849: The deleteTransferWebhook function is now a no-op
returning nil, so remove its dead call and associated error handling/comment in
processPaymentOrderPostCreation (references: deleteTransferWebhook and
processPaymentOrderPostCreation) and update the doc comment on
processPaymentOrderPostCreation that still mentions "webhook deletion"; either
delete both the function and the call site or remove the call plus stale
inline/doc comments so no unreachable error-guard remains. Before removing or
changing lifecycle behavior in services/common/order.go, raise the change for
discussion per the repo guideline about altering order lifecycle/refund flows.
In `@services/indexer/evm.go`:
- Around line 308-320: Update the stale doc comments to remove any mention of
the removed "engine" fallback and accurately describe current behavior: for
getAddressTransactionHistoryWithFallback, say it tries Etherscan first and falls
back to Blockscout; for getAddressTransactionHistoryImmediate, say it tries
Etherscan immediately (bypassing queue) and falls back to Blockscout; and for
getAddressTransactionHistoryWithFallbackAndBypass update the header to reflect
Etherscan with optional bypass and Blockscout fallback. Use the function names
getAddressTransactionHistoryWithFallback, getAddressTransactionHistoryImmediate,
and getAddressTransactionHistoryWithFallbackAndBypass to locate and update the
comments.
In `@services/indexer/rpc_events.go`:
- Around line 93-108: Replace the untyped map event construction with a concrete
struct (e.g., type RPCEvent struct { BlockNumber uint64; TransactionHash string;
LogIndex uint; Address string; Topics []common.Hash; Data []byte; Decoded struct
{ IndexedParams map[string]interface{} `json:"indexed_params"`; NonIndexedParams
map[string]interface{} `json:"non_indexed_params"` } }) and use a slice of that
struct instead of events []interface{}; update the creation site (the loop over
logs) to populate the struct fields (match names/types used by
ProcessRPCEventsBySignature) and adjust any downstream consumers (or convert the
slice to []interface{} only at the boundary) so ProcessRPCEventsBySignature
receives strongly typed data instead of map[string]interface{}.
In `@services/order/evm.go`:
- Line 272: There are TODO notes left about setting the transaction hash
directly instead of invoking IndexGateway; create tracked follow-up issues to
replace the direct assignment with a call to IndexGateway(txHash) and wire the
call where the TODOs exist (ensure IndexGateway is invoked with the tx hash
rather than writing it directly), add unit/integration tests asserting
IndexGateway is called and the txHash flow works, update error handling and
logging around the IndexGateway call, and remove the TODOs once implemented;
include acceptance criteria in each issue (call made, retries/errors handled,
tests passing).
In `@tasks/refunds.go`:
- Around line 186-193: The ERC20 ABI is being parsed inside each goroutine using
abi.JSON(strings.NewReader(contracts.ERC20TokenMetaData.ABI)), causing redundant
work; parse contracts.ERC20TokenMetaData.ABI once before spawning the per-order
goroutines (e.g., create a single erc20ABI via abi.JSON and handle its error
upfront) and then reuse that erc20ABI inside the loop/goroutines (referencing
the erc20ABI symbol) so each goroutine does not reparse the ABI.
In `@utils/eip7702.go`:
- Around line 118-128: Replace the hardcoded gasLimit in SendKeeperTx with a
dynamic estimate: call client.EstimateGas using an ethereum.CallMsg built from
the keeper address (derive keeperAddr from keeperKey), To: &to and Data: data,
handle estimation errors, then set gasLimit to the estimated value plus a small
safety margin (e.g., 20%), and still ensure gasLimit fits uint64; keep the
existing fallback behavior only if estimation fails.
In `@utils/nonce_manager_test.go`:
- Around line 234-275: Update the test to exercise the SubmitWithNonce retry
loop by invoking SubmitWithNonce (not just
AcquireNonce/isNonceTooLow/ResetNonce) with a submitFn that fails with a
nonce-too-low error on the first invocation and succeeds on the second;
implement this by using a counter or goroutine to reseed the NonceManager.nonces
(or call ResetNonce and then set nm.nonces[nonceKey{chainID, addr}] =
<newNonce>) between attempts so SubmitWithNonce triggers a reset and retry path,
and assert that SubmitWithNonce ultimately returns success and used the
re-seeded nonce. Ensure you reference SubmitWithNonce, submitFn, isNonceTooLow,
ResetNonce, and AcquireNonce in the test to locate and modify the behavior.
In `@utils/nonce_manager.go`:
- Around line 104-111: The isNonceTooLow function currently relies on fragile
string-matching against error messages; update it by adding a clear comment
above isNonceTooLow that lists which clients each matched substring corresponds
to (e.g., Geth, Erigon, Nethermind, OpenEthereum) and note that this list must
be expanded when onboarding new RPC providers, and then factor the literals into
named constants or a slice (e.g., nonceErrSignatures) so future additions are
obvious and centralized; ensure the comment also advises preferencing structured
error codes if/when providers expose them.
- Around line 31-48: AcquireNonce currently holds nm.mu while calling
client.PendingNonceAt which blocks all AcquireNonce callers; change to fetch
pendingNonce outside the global lock using a double-check or per-key lock: first
lock nm.mu and check nm.nonces[key], if missing release the lock, call
client.PendingNonceAt(ctx, addr) to get pendingNonce, then re-lock nm.mu and
check nm.nonces[key] again (if still missing set nm.nonces[key]=pendingNonce),
then increment and return the nonce; reference functions/types: AcquireNonce,
NonceManager.mu, nonceKey, nm.nonces, and client.PendingNonceAt.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (25)
config/crypto.gocontrollers/sender/sender.goent/migrate/schema.goent/mutation.goent/network.goent/network/network.goent/network/where.goent/network_create.goent/network_update.goent/runtime/runtime.goent/schema/network.gogo.modmain.goservices/common/order.goservices/engine.goservices/engine_test.goservices/indexer/evm.goservices/indexer/rpc_events.goservices/order/evm.goservices/receive_address.gotasks/refunds.goutils/eip7702.goutils/eip7702_test.goutils/nonce_manager.goutils/nonce_manager_test.go
💤 Files with no reviewable changes (3)
- services/engine_test.go
- main.go
- services/engine.go
- Added error handling for empty receive address salt in CreateOrder method. - Updated CreateOrder, RefundOrder, and SettleOrder methods to check transaction receipt status after sending transactions, ensuring proper error reporting for reverted transactions. - Improved delegation check logic in utils/eip7702.go to validate delegation contract address more accurately. - Refactored SendKeeperTx to handle context cancellation during transaction receipt retrieval, enhancing robustness in transaction management. - Added Close method to RPCClient interface for proper resource management.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
services/indexer/rpc_events.go (1)
17-22: VerifyRPCClient.Close()interface signature is correct.
defer client.Close()looks good, and the commit message confirmsClose()was added to theRPCClientinterface. However,ethclient.Client.Close()has no return value — if the interface was added asClose() error(as the past review suggested), theethRPCstruct embedding*ethclient.Clientwould not satisfy the interface and the build would fail. The correct declaration isClose()with no return.Run to confirm the interface signature:
#!/bin/bash # Verify RPCClient interface includes Close() with correct signature (no error return) rg -n 'type RPCClient interface' types/types.go -A 20 rg -n 'func.*Close' types/types.go -A 2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/indexer/rpc_events.go` around lines 17 - 22, The RPCClient interface signature must declare Close() with no return value so ethRPC (which embeds *ethclient.Client) satisfies it; verify and if needed change the declaration in type RPCClient to "Close()" (no error) and update any implementations or callers that assumed "Close() error" (e.g., remove error handling around client.Close() in GetContractEventsRPC where defer client.Close() is used); confirm ethRPC struct embeds *ethclient.Client and runs go build/tests (or run the provided rg checks) to ensure the interface now matches the underlying ethclient.Client Close() signature.tasks/refunds.go (1)
237-270: Status update after successful refund is now correctly implemented.The previous review flagged the missing order-status update after a successful keeper transaction. The current code correctly updates
StatusRefunded,TxHash, andBlockNumber(lines 260–264) before returning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tasks/refunds.go` around lines 237 - 270, Previously missing order status update after a successful keeper transaction has been added: ensure that after utils.DefaultNonceManager.SubmitWithNonce calls utils.SendKeeperTx and receipt.Status != 0, you update the order via order.Update().SetStatus(paymentorder.StatusRefunded).SetTxHash(receipt.TxHash.Hex()).SetBlockNumber(receipt.BlockNumber).Save(ctx); keep the existing error logging for Save failures and the existing logging on SubmitWithNonce failures and reverted receipts so the code path around SubmitWithNonce, SendKeeperTx, receipt handling and order.Update() remains consistent.
🧹 Nitpick comments (2)
utils/eip7702.go (1)
133-134: HardcodedgasLimit = 500_000may silently OOG complex batches.A static limit works for today's simple 2-call batches but will revert with out-of-gas if calldata grows, gas repricing occurs, or additional calls are added to the batch. Consider using
client.EstimateGaswith a safety multiplier as the primary estimate, falling back to the cap only on estimation failure.♻️ Proposed refactor
- gasLimit := uint64(500_000) + gasLimit := uint64(500_000) // cap + if estimated, err := client.EstimateGas(ctx, ethereum.CallMsg{ + From: crypto.PubkeyToAddress(keeperKey.PublicKey), + To: &to, + Data: data, + Value: big.NewInt(0), + }); err == nil { + gasLimit = estimated * 12 / 10 // 20% headroom + if gasLimit > 500_000 { + gasLimit = 500_000 + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/eip7702.go` around lines 133 - 134, The hardcoded gasLimit (gasLimit := uint64(500_000)) can cause out-of-gas for larger/changed batches; replace it by estimating gas via the RPC client (use client.EstimateGas or ethclient.EstimateGas for the same call payload used when constructing tx in the function that sets gasFeeCap/gasLimit) and apply a safety multiplier (e.g., 1.2–1.5) to that estimate, and only use the hard cap as a fallback if EstimateGas fails or returns zero; ensure this logic lives near where gasFeeCap and gasLimit are computed so the tx uses the estimated/sanitized gasLimit.services/order/evm.go (1)
182-186: Aggregator key is re-parsed from config string on everyCreateOrder,RefundOrder, andSettleOrdercall.
crypto.HexToECDSAis called at lines 182–185, 248–251, and 319–322. Cache it once in theOrderEVMstruct (or at package init) to avoid the repeated hex-decode + key-derivation work on each request.♻️ Proposed refactor
type OrderEVM struct { priorityQueue *services.PriorityQueueService nonceManager *utils.NonceManager + aggregatorKey *ecdsa.PrivateKey + aggregatorAddr ethcommon.Address } func NewOrderEVM() orderTypes.OrderService { + key, err := crypto.HexToECDSA(cryptoConf.AggregatorEVMPrivateKey) + if err != nil { + panic(fmt.Sprintf("NewOrderEVM: invalid aggregator key: %v", err)) + } return &OrderEVM{ priorityQueue: services.NewPriorityQueueService(), nonceManager: utils.DefaultNonceManager, + aggregatorKey: key, + aggregatorAddr: crypto.PubkeyToAddress(key.PublicKey), } }Then replace the three inline
crypto.HexToECDSA/crypto.PubkeyToAddressblocks inCreateOrder,RefundOrder, andSettleOrderwiths.aggregatorKey/s.aggregatorAddr.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/order/evm.go` around lines 182 - 186, The code repeatedly calls crypto.HexToECDSA and derives an address in CreateOrder, RefundOrder, and SettleOrder; add cached fields to the OrderEVM struct (e.g., aggregatorKey *ecdsa.PrivateKey and aggregatorAddr common.Address), initialize them once (in the OrderEVM constructor/new function or package init) by parsing cryptoConf.AggregatorEVMPrivateKey and computing crypto.PubkeyToAddress(aggregatorKey.PublicKey), return/init an error if parse fails, and then replace the inline crypto.HexToECDSA/crypto.PubkeyToAddress usages in CreateOrder, RefundOrder, and SettleOrder with references to s.aggregatorKey and s.aggregatorAddr.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/indexer/rpc_events.go`:
- Around line 55-60: Add an explicit validation that toBlock >= fromBlock in the
RPC event range checks (the block-range validation near fromBlock, toBlock and
eventSignatures using utils.TransferEventSignature) and return a clear error if
not; update the guard before the range-size checks so that an inverted range
cannot bypass the toBlock-fromBlock comparisons and later cause FilterLogs to
receive an invalid range.
In `@tasks/refunds.go`:
- Around line 64-68: The current query for expiredOrders uses
paymentorder.CreatedAtGTE(time.Now().Add(-(RefundsInterval * time.Minute)))
which will permanently skip expired orders created before that window; remove
the CreatedAtGTE filter and only filter by
paymentorder.StatusEQ(paymentorder.StatusExpired) in the
storage.Client.PaymentOrder.Query() so all expired orders are returned (the
refund workflow is idempotent because successful refunds set StatusRefunded).
Update any tests or comments referencing RefundsInterval as needed.
In `@utils/eip7702.go`:
- Around line 74-97: The SignBatch7702 function currently omits chain ID,
ignores signerAddr, and uses int64 conversion for nonce which can overflow;
change the signature of SignBatch7702 to accept chainID *big.Int, use
new(big.Int).SetUint64(nonce) (instead of big.NewInt(int64(nonce))), and include
both chainID and signerAddr in the signed digest (e.g., append chainID.Bytes()
and signerAddr.Bytes() into the packed message before hashing) so the signature
is bound to chain and EOA; then update all call sites (e.g., in
services/order/evm.go and tasks/refunds.go) to pass the existing chainID value
to SignBatch7702.
---
Duplicate comments:
In `@services/indexer/rpc_events.go`:
- Around line 17-22: The RPCClient interface signature must declare Close() with
no return value so ethRPC (which embeds *ethclient.Client) satisfies it; verify
and if needed change the declaration in type RPCClient to "Close()" (no error)
and update any implementations or callers that assumed "Close() error" (e.g.,
remove error handling around client.Close() in GetContractEventsRPC where defer
client.Close() is used); confirm ethRPC struct embeds *ethclient.Client and runs
go build/tests (or run the provided rg checks) to ensure the interface now
matches the underlying ethclient.Client Close() signature.
In `@tasks/refunds.go`:
- Around line 237-270: Previously missing order status update after a successful
keeper transaction has been added: ensure that after
utils.DefaultNonceManager.SubmitWithNonce calls utils.SendKeeperTx and
receipt.Status != 0, you update the order via
order.Update().SetStatus(paymentorder.StatusRefunded).SetTxHash(receipt.TxHash.Hex()).SetBlockNumber(receipt.BlockNumber).Save(ctx);
keep the existing error logging for Save failures and the existing logging on
SubmitWithNonce failures and reverted receipts so the code path around
SubmitWithNonce, SendKeeperTx, receipt handling and order.Update() remains
consistent.
---
Nitpick comments:
In `@services/order/evm.go`:
- Around line 182-186: The code repeatedly calls crypto.HexToECDSA and derives
an address in CreateOrder, RefundOrder, and SettleOrder; add cached fields to
the OrderEVM struct (e.g., aggregatorKey *ecdsa.PrivateKey and aggregatorAddr
common.Address), initialize them once (in the OrderEVM constructor/new function
or package init) by parsing cryptoConf.AggregatorEVMPrivateKey and computing
crypto.PubkeyToAddress(aggregatorKey.PublicKey), return/init an error if parse
fails, and then replace the inline crypto.HexToECDSA/crypto.PubkeyToAddress
usages in CreateOrder, RefundOrder, and SettleOrder with references to
s.aggregatorKey and s.aggregatorAddr.
In `@utils/eip7702.go`:
- Around line 133-134: The hardcoded gasLimit (gasLimit := uint64(500_000)) can
cause out-of-gas for larger/changed batches; replace it by estimating gas via
the RPC client (use client.EstimateGas or ethclient.EstimateGas for the same
call payload used when constructing tx in the function that sets
gasFeeCap/gasLimit) and apply a safety multiplier (e.g., 1.2–1.5) to that
estimate, and only use the hard cap as a fallback if EstimateGas fails or
returns zero; ensure this logic lives near where gasFeeCap and gasLimit are
computed so the tx uses the estimated/sanitized gasLimit.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
services/indexer/rpc_events.goservices/order/evm.gotasks/refunds.gotypes/types.goutils/eip7702.go
…ransaction management - Added EngineService to handle interactions with the thirdweb API, including methods for creating wallets, fetching the latest block, and managing transactions. - Introduced ParseUserOpErrorJSON method to parse and handle user operation errors from JSON responses. - Implemented unit tests for error parsing to ensure correct identification of error types. - Enhanced transaction management with methods for sending transaction batches and checking transaction statuses.
…tion history - Added EngineService to IndexerEVM for improved handling of contract events and transaction history retrieval. - Replaced direct RPC calls with EngineService methods in indexReceiveAddressByTransaction and indexGatewayByTransaction for better error handling and fallback mechanisms. - Removed the deprecated rpc_events.go file as its functionality is now encapsulated within EngineService. - Updated transaction history retrieval logic to utilize EngineService, ensuring support for additional chains while maintaining compatibility with existing services.
onahprosper
changed the title
onahprosper
changed the title
- Updated the error handling in the AssignPaymentOrder method to return a more descriptive error message when no provider matches the order, enhancing clarity in error reporting.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
tasks/refunds.go (1)
64-69:⚠️ Potential issue | 🟠 MajorRemove the
CreatedAtGTEwindow from expired-refund selection.Filtering expired orders by creation time (Line 68) can permanently skip older orders that are still
StatusExpired.🐛 Proposed fix
expiredOrders, err := storage.Client.PaymentOrder. Query(). Where( paymentorder.StatusEQ(paymentorder.StatusExpired), - paymentorder.CreatedAtGTE(time.Now().Add(-(RefundsInterval * time.Minute))), ).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tasks/refunds.go` around lines 64 - 69, The query that builds expiredOrders currently applies paymentorder.CreatedAtGTE(time.Now().Add(-(RefundsInterval * time.Minute))) which incorrectly restricts expired refunds to a time window; remove that predicate so the Where(...) call only filters by paymentorder.StatusEQ(paymentorder.StatusExpired). Locate the storage.Client.PaymentOrder.Query().Where(...) invocation and delete the paymentorder.CreatedAtGTE(...) clause (and any related comma/formatting) so all orders with StatusExpired are selected regardless of creation time.utils/eip7702.go (1)
74-97:⚠️ Potential issue | 🟠 MajorBind
SignBatch7702signatures to chain + signer to prevent replay.At Line 87, the digest excludes both
chainIDandsignerAddr, so a valid signature can be replayed on another chain (andsignerAddris currently ineffective).🔐 Proposed fix
-func SignBatch7702(privateKey *ecdsa.PrivateKey, signerAddr common.Address, nonce uint64, calls []Call7702) ([]byte, error) { +func SignBatch7702(privateKey *ecdsa.PrivateKey, signerAddr common.Address, chainID *big.Int, nonce uint64, calls []Call7702) ([]byte, error) { + if chainID == nil { + return nil, fmt.Errorf("chainID is required") + } var packed []byte for _, c := range calls { packed = append(packed, c.To.Bytes()...) valBytes := make([]byte, 32) c.Value.FillBytes(valBytes) packed = append(packed, valBytes...) packed = append(packed, c.Data...) } nonceBytes := make([]byte, 32) new(big.Int).SetUint64(nonce).FillBytes(nonceBytes) - digest := crypto.Keccak256Hash(append(nonceBytes, packed...)) + chainIDBytes := make([]byte, 32) + chainID.FillBytes(chainIDBytes) + digest := crypto.Keccak256Hash( + append(append(append(chainIDBytes, signerAddr.Bytes()...), nonceBytes...), packed...), + ) prefix := []byte("\x19Ethereum Signed Message:\n32") msgHash := crypto.Keccak256Hash(append(prefix, digest.Bytes()...))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/eip7702.go` around lines 74 - 97, The signature digest in SignBatch7702 is missing chainID and signer address, enabling cross-chain replay; update the function signature to accept chainID (uint64 or *big.Int) and include signerAddr.Bytes() plus a 32-byte representation of chainID into the data used to compute digest (e.g., append chainIDBytes and signerAddr.Bytes() into the sequence before nonce/packed or before computing digest), then compute digest = Keccak256Hash( ...nonceBytes, chainIDBytes, signerAddr.Bytes(), packed...) so signatures are bound to both the chain and signer; keep the rest of the flow (msg prefix, crypto.Sign, v adjustment) unchanged and ensure chainID is converted to a 32-byte big-endian slice (like nonceBytes).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tasks/refunds.go`:
- Around line 133-142: After deriving userAddr from the decrypted salt (in the
block that calls crypto.HexToECDSA and crypto.PubkeyToAddress), verify userAddr
matches order.ReceiveAddress before proceeding to sign/submit transactions; if
they differ, log an error with order.ID and both addresses (or their hex
strings) and return to abort processing. Ensure the comparison handles the
address types used (e.g., compare common.Address values or their Hex() strings)
and place this check immediately after the userAddr calculation.
- Around line 99-105: Validate hex addresses with ethcommon.IsHexAddress()
before any ethcommon.HexToAddress() conversions to avoid silent coercion: check
order.ReturnAddress and order.ReceiveAddress (and their salts) before building
transfers, validate tokenContract (tokenContract address string) before
constructing ERC20 calls, and validate network.DelegationContractAddress before
delegation checks; if IsHexAddress returns false, return an error/log and skip
the operation. Locate HexToAddress usages around variables order.ReturnAddress,
tokenContract, and network.DelegationContractAddress and guard each with
IsHexAddress() checks, failing fast with a clear error message when validation
fails.
In `@utils/eip7702.go`:
- Around line 129-134: Guard against a nil BaseFee on the retrieved header
before doing math: check if head.BaseFee != nil and only compute gasFeeCap via
new(big.Int).Add(new(big.Int).Mul(head.BaseFee, big.NewInt(2)), gasTipCap) when
BaseFee is present; otherwise set gasFeeCap to a safe fallback (e.g., gasTipCap
or gasTipCap plus zero) to avoid dereferencing nil. Update the code around the
client.HeaderByNumber call and the gasFeeCap assignment in utils/eip7702.go
(references: head.BaseFee, gasFeeCap) to follow the same pattern used in
utils/userop.go.
---
Duplicate comments:
In `@tasks/refunds.go`:
- Around line 64-69: The query that builds expiredOrders currently applies
paymentorder.CreatedAtGTE(time.Now().Add(-(RefundsInterval * time.Minute)))
which incorrectly restricts expired refunds to a time window; remove that
predicate so the Where(...) call only filters by
paymentorder.StatusEQ(paymentorder.StatusExpired). Locate the
storage.Client.PaymentOrder.Query().Where(...) invocation and delete the
paymentorder.CreatedAtGTE(...) clause (and any related comma/formatting) so all
orders with StatusExpired are selected regardless of creation time.
In `@utils/eip7702.go`:
- Around line 74-97: The signature digest in SignBatch7702 is missing chainID
and signer address, enabling cross-chain replay; update the function signature
to accept chainID (uint64 or *big.Int) and include signerAddr.Bytes() plus a
32-byte representation of chainID into the data used to compute digest (e.g.,
append chainIDBytes and signerAddr.Bytes() into the sequence before nonce/packed
or before computing digest), then compute digest = Keccak256Hash( ...nonceBytes,
chainIDBytes, signerAddr.Bytes(), packed...) so signatures are bound to both the
chain and signer; keep the rest of the flow (msg prefix, crypto.Sign, v
adjustment) unchanged and ensure chainID is converted to a 32-byte big-endian
slice (like nonceBytes).
…er entities - Introduced `sponsorship_mode` field to the `Network` entity, allowing for options like "thirdweb" and "self_sponsored". - Added `wallet_type` field to the `PaymentOrder` entity, supporting types such as "smart_wallet" and "eoa_7702". - Updated relevant mutation, creation, and update methods to handle the new fields. - Enhanced database schema and migration scripts to accommodate these changes, ensuring backward compatibility and data integrity.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (5)
tasks/refunds.go (4)
184-184: Assert deriveduserAddrmatchesorder.ReceiveAddressbefore signing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tasks/refunds.go` at line 184, Before signing, verify that the derived address from the user's public key (userAddr computed via crypto.PubkeyToAddress(userKey.PublicKey)) equals the expected recipient address (order.ReceiveAddress); if it does not match, return an error or abort the refund flow rather than proceeding to the signing step (e.g., in the function performing the refund/signing). Add this explicit check immediately after the userAddr assignment and before any call to the signing logic to prevent signing funds for the wrong address.
66-71:CreatedAtGTEfilter permanently silences old expired orders.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tasks/refunds.go` around lines 66 - 71, The CreatedAtGTE filter on storage.Client.PaymentOrder.Query is wrong—using CreatedAtGTE(time.Now().Add(-(RefundsInterval * time.Minute))) only selects recently created expired orders and permanently ignores older ones; replace that predicate with CreatedAtLTE(time.Now().Add(-(RefundsInterval * time.Minute))) (or remove the created-at bound if the intent is to process all expired orders) so paymentorder.CreatedAt comparison uses LTE instead of GTE; update the query where clause referencing paymentorder.CreatedAtGTE to paymentorder.CreatedAtLTE and keep the same time expression (using RefundsInterval) to select orders created before the cutoff.
252-271:receiptcaptured via closure acrossSubmitWithNoncecall — nonce accounting issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tasks/refunds.go` around lines 252 - 271, receipt is being captured by the outer variable inside the SubmitWithNonce closure which breaks nonce retry accounting; change the pattern so SubmitWithNonce returns the receipt instead of relying on closure-capture. Update utils.DefaultNonceManager.SubmitWithNonce to have signature that returns (*ethTypes.Receipt, error) and accept a callback func(nonce uint64) (*ethTypes.Receipt, error) (or add a separate SubmitWithNonceWithResult wrapper) and then call it as receipt, err := utils.DefaultNonceManager.SubmitWithNonce(ctx, client, network.ChainID, aggregatorAddr, func(nonce uint64) (*ethTypes.Receipt, error) { return utils.SendKeeperTx(ctx, client, aggregatorKey, nonce, userAddr, batchData, authList, chainID) }); remove the outer receipt variable capture and adjust error handling to use the returned receipt.
170-184: Validate hex addresses withIsHexAddressbeforeHexToAddressconversions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tasks/refunds.go` around lines 170 - 184, The decrypted salt (order.ReceiveAddressSalt via cryptoUtils.DecryptPlain) is being passed to crypto.HexToECDSA and then converted to an address via crypto.PubkeyToAddress without validating it's a proper hex value; validate inputs first: after saltDecrypted, trim any "0x" prefix and ensure the string contains only hex chars and correct length for a private key (or use an existing validator), then call crypto.HexToECDSA; additionally, anywhere you convert hex strings to addresses using common.HexToAddress, first call common.IsHexAddress and return an error if it fails; update the handlers around crypto.HexToECDSA, crypto.PubkeyToAddress, and any HexToAddress usage to include these checks.services/order/evm.go (1)
227-241:receiptcaptured via closure acrossSubmitWithNonce— nonce accounting issue on revert.Also applies to: 336-350, 439-453
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/order/evm.go` around lines 227 - 241, The code captures receipt via the outer variable while calling nonceManager.SubmitWithNonce with a closure, which can cause wrong nonce/accounting if the tx reverts; change the pattern so the receipt is returned from the SubmitWithNonce call (or have the callback return (*types.Receipt, error) instead of only error) and perform the revert check using that returned receipt inside the SubmitWithNonce caller; specifically update nonceManager.SubmitWithNonce usage and the callback that calls utils.SendKeeperTx (and similar sites using SubmitWithNonce) so the callback yields the receipt directly (or propagate it through the SubmitWithNonce return value) and then check receipt.Status immediately after obtaining the returned receipt rather than relying on a closure-captured variable.
🧹 Nitpick comments (4)
services/order/evm.go (2)
113-116: Open@todocomments for discarded result maps across all three operations.
createOrderFor7702,refundOrderFor7702, andsettleOrderFor7702each return amap[string]interface{}withtransactionHashandblockNumber, but all three call sites discard the result (_, err = ...) with@todomarkers. If persisting on-chain references for observability or idempotency is a near-term requirement, these should be tracked.Would you like me to open a tracking issue or generate the persistence logic for storing the transaction hash/block number on the order entity for the 7702 paths?
Also applies to: 284-287, 386-390
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/order/evm.go` around lines 113 - 116, The calls to createOrderFor7702, refundOrderFor7702, and settleOrderFor7702 currently discard their returned map (which contains transactionHash and blockNumber) — capture and persist those on the order record for observability/idempotency instead of using "_, err = ..."; modify the call sites that call createOrderFor7702, refundOrderFor7702, and settleOrderFor7702 to assign the returned map (e.g., txInfo, err := ...), extract txInfo["transactionHash"] and txInfo["blockNumber"], and update the order entity (or call an existing order persistence method) to store those fields (or add them to the order struct if missing), handling nil/absent keys and errors before proceeding.
221-235:aggregatorKeyre-parsed on every 7702 helper call; move to struct initialization.
crypto.HexToECDSA(cryptoConf.AggregatorEVMPrivateKey)is called identically at the top of each of the three 7702 helper functions (createOrderFor7702,refundOrderFor7702,settleOrderFor7702). This is pure duplication — the key is immutable at runtime. Parse it once inNewOrderEVM()and store it on the struct alongsidenonceManager.♻️ Proposed refactor
type OrderEVM struct { priorityQueue *services.PriorityQueueService engineService *services.EngineService nonceManager *utils.NonceManager + aggregatorKey *ecdsa.PrivateKey + aggregatorAddr ethcommon.Address } func NewOrderEVM() orderTypes.OrderService { + aggregatorKey, err := crypto.HexToECDSA(config.CryptoConfig().AggregatorEVMPrivateKey) + if err != nil { + panic(fmt.Sprintf("NewOrderEVM: invalid AggregatorEVMPrivateKey: %v", err)) + } return &OrderEVM{ priorityQueue: services.NewPriorityQueueService(), engineService: services.NewEngineService(), nonceManager: utils.DefaultNonceManager, + aggregatorKey: aggregatorKey, + aggregatorAddr: crypto.PubkeyToAddress(aggregatorKey.PublicKey), } }Then replace the three inline key-parse blocks with
s.aggregatorKeyands.aggregatorAddr.Also applies to: 323-344, 426-447
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/order/evm.go` around lines 221 - 235, Parse the aggregator ECDSA key once in NewOrderEVM and store both the parsed private key and derived address on the service struct (e.g., add fields s.aggregatorKey and s.aggregatorAddr alongside s.nonceManager), then remove the repeated crypto.HexToECDSA/crypto.PubkeyToAddress calls from createOrderFor7702, refundOrderFor7702, and settleOrderFor7702 and use s.aggregatorKey and s.aggregatorAddr instead; ensure NewOrderEVM returns an error if HexToECDSA fails so callers handle initialization errors and update any error messages in the helper functions to reference the stored fields rather than re-parsing.tasks/refunds.go (1)
127-129: Open@todofor 7702 refund response handling.The
@todoat line 128 ("will decide how to handle the response later") is left unresolved. The returnedmap[string]interface{}containingtransactionHashandblockNumberis currently discarded.Would you like me to open a tracking issue for storing the refund transaction hash/block number on the order entity, or should it be addressed in this PR?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tasks/refunds.go` around lines 127 - 129, The refund response from refundExpiredOrderFor7702 is currently discarded; capture its returned map (transactionHash, blockNumber) and persist those values onto the order record (e.g., add/assign RefundTransactionHash and RefundBlockNumber on the order struct if absent) then call the existing order save/update function (e.g., UpdateOrder, SaveOrder, or orderRepo.Save) to persist changes; remove the `@todo` comment. Ensure refundExpiredOrderFor7702(ctx, order, ...) return value is checked for nil and handle extraction of "transactionHash" and "blockNumber" keys safely before saving.main.go (1)
58-62: Add retry/observability around non-fatal gateway webhook bootstrap failure.Since startup now continues on failure (Line 60), consider a background retry with metrics/alerts so thirdweb-mode networks don’t degrade silently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.go` around lines 58 - 62, The CreateGatewayWebhook call in main (engineService.CreateGatewayWebhook) currently logs a non-fatal warning and continues; change this to spawn a background retry loop that retries CreateGatewayWebhook with exponential backoff (or limited retries) until success, and emit observability signals (increment a metric/counter and emit a warning/error via your metrics/telemetry API on each failure and a success event when it succeeds) so failures are visible; ensure the retry runs as a goroutine and use the existing logger and metrics client to report attempts, backoff, final failure (if you cap retries), and eventual success.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controllers/sender/sender.go`:
- Around line 424-425: Call CreateSmartAddress with the request context variable
(reqCtx) instead of the Gin context (ctx/*gin.Context) for the I/O helper
invocations; update the two places where you call
receiveAddressService.CreateSmartAddress (the line with "address, salt, err :=
ctrl.receiveAddressService.CreateSmartAddress(ctx, label, mode)" and the similar
call around lines 1063-1064) to pass reqCtx so the helper receives a
context.Context for cancellation/timeouts.
In
`@ent/migrate/migrations/20260225165800_add_sponsorship_mode_and_wallet_type.sql`:
- Around line 1-4: The migration is missing the
networks.delegation_contract_address column declared in ent/schema/network.go;
update the SQL migration (in the
20260225165800_add_sponsorship_mode_and_wallet_type.sql diff) to ALTER TABLE
"networks" ADD COLUMN "delegation_contract_address" character varying (match
nullability from ent/schema/network.go — likely nullable) so the DB schema
matches the Network field declaration.
In `@ent/mutation.go`:
- Around line 8721-8724: The slice capacity in PaymentOrderMutation.Fields() is
undersized (make([]string, 0, 37)) while 67 fields are appended; update the
capacity from 37 to 67 to avoid repeated reallocations—locate the Fields()
method on type PaymentOrderMutation and change the third argument of
make([]string, 0, 37) to 67 so the slice is preallocated to the correct size.
In `@ent/paymentorder/paymentorder.go`:
- Around line 357-364: The default wallet_type constant (DefaultWalletType =
WalletTypeSmartWallet) causes silent fallback for any PaymentOrder create that
omits SetWalletType; remove this latent risk by requiring explicit wallet_type
on creation: either remove or clear the default in the
ent/paymentorder_create.go builder and add validation in the PaymentOrder
creation flow (e.g., a BeforeCreate/Validate hook or in the Save path) that
checks the wallet_type field is set to one of WalletTypeSmartWallet or
WalletTypeEoa7702 and returns an error if blank; ensure callers use
SetWalletType(walletType) (reference SetWalletType, Save, DefaultWalletType,
WalletTypeSmartWallet, WalletTypeEoa7702) so all current and future create paths
must explicitly provide wallet_type.
In `@ent/schema/network.go`:
- Around line 32-35: Add a DB-level CHECK that prevents rows in the networks
table from having sponsorship_mode == "self_sponsored" while
delegation_contract_address is empty: create a constraint named
networks_self_sponsored_requires_delegation_contract (or equivalent) that
enforces (sponsorship_mode <> 'self_sponsored' OR delegation_contract_address <>
''); implement this by updating the ent schema/migration for the networks entity
(the fields delegation_contract_address and sponsorship_mode) so the generated
migration adds the CHECK constraint and ensure the migration is run/applied.
In `@tasks/refunds.go`:
- Around line 124-148: The code sets paymentorder.StatusRefunded immediately
after calling refundExpiredOrderForSmartWallet even though
refundExpiredOrderForSmartWallet/Engine.SendContractCall is fire-and-forget;
change the behavior to mirror services/order/evm.go: set the intermediate status
paymentorder.StatusRefunding (instead of StatusRefunded) for the
WalletTypeSmartWallet path (where refundExpiredOrderForSmartWallet is used),
persist that via order.Update().SetStatus(...) and let the Engine/confirmation
callback (or the same retry/confirm flow used by RefundOrder) set StatusRefunded
only after on-chain confirmation or a successful Engine job completion.
---
Duplicate comments:
In `@services/order/evm.go`:
- Around line 227-241: The code captures receipt via the outer variable while
calling nonceManager.SubmitWithNonce with a closure, which can cause wrong
nonce/accounting if the tx reverts; change the pattern so the receipt is
returned from the SubmitWithNonce call (or have the callback return
(*types.Receipt, error) instead of only error) and perform the revert check
using that returned receipt inside the SubmitWithNonce caller; specifically
update nonceManager.SubmitWithNonce usage and the callback that calls
utils.SendKeeperTx (and similar sites using SubmitWithNonce) so the callback
yields the receipt directly (or propagate it through the SubmitWithNonce return
value) and then check receipt.Status immediately after obtaining the returned
receipt rather than relying on a closure-captured variable.
In `@tasks/refunds.go`:
- Line 184: Before signing, verify that the derived address from the user's
public key (userAddr computed via crypto.PubkeyToAddress(userKey.PublicKey))
equals the expected recipient address (order.ReceiveAddress); if it does not
match, return an error or abort the refund flow rather than proceeding to the
signing step (e.g., in the function performing the refund/signing). Add this
explicit check immediately after the userAddr assignment and before any call to
the signing logic to prevent signing funds for the wrong address.
- Around line 66-71: The CreatedAtGTE filter on
storage.Client.PaymentOrder.Query is wrong—using
CreatedAtGTE(time.Now().Add(-(RefundsInterval * time.Minute))) only selects
recently created expired orders and permanently ignores older ones; replace that
predicate with CreatedAtLTE(time.Now().Add(-(RefundsInterval * time.Minute)))
(or remove the created-at bound if the intent is to process all expired orders)
so paymentorder.CreatedAt comparison uses LTE instead of GTE; update the query
where clause referencing paymentorder.CreatedAtGTE to paymentorder.CreatedAtLTE
and keep the same time expression (using RefundsInterval) to select orders
created before the cutoff.
- Around line 252-271: receipt is being captured by the outer variable inside
the SubmitWithNonce closure which breaks nonce retry accounting; change the
pattern so SubmitWithNonce returns the receipt instead of relying on
closure-capture. Update utils.DefaultNonceManager.SubmitWithNonce to have
signature that returns (*ethTypes.Receipt, error) and accept a callback
func(nonce uint64) (*ethTypes.Receipt, error) (or add a separate
SubmitWithNonceWithResult wrapper) and then call it as receipt, err :=
utils.DefaultNonceManager.SubmitWithNonce(ctx, client, network.ChainID,
aggregatorAddr, func(nonce uint64) (*ethTypes.Receipt, error) { return
utils.SendKeeperTx(ctx, client, aggregatorKey, nonce, userAddr, batchData,
authList, chainID) }); remove the outer receipt variable capture and adjust
error handling to use the returned receipt.
- Around line 170-184: The decrypted salt (order.ReceiveAddressSalt via
cryptoUtils.DecryptPlain) is being passed to crypto.HexToECDSA and then
converted to an address via crypto.PubkeyToAddress without validating it's a
proper hex value; validate inputs first: after saltDecrypted, trim any "0x"
prefix and ensure the string contains only hex chars and correct length for a
private key (or use an existing validator), then call crypto.HexToECDSA;
additionally, anywhere you convert hex strings to addresses using
common.HexToAddress, first call common.IsHexAddress and return an error if it
fails; update the handlers around crypto.HexToECDSA, crypto.PubkeyToAddress, and
any HexToAddress usage to include these checks.
---
Nitpick comments:
In `@main.go`:
- Around line 58-62: The CreateGatewayWebhook call in main
(engineService.CreateGatewayWebhook) currently logs a non-fatal warning and
continues; change this to spawn a background retry loop that retries
CreateGatewayWebhook with exponential backoff (or limited retries) until
success, and emit observability signals (increment a metric/counter and emit a
warning/error via your metrics/telemetry API on each failure and a success event
when it succeeds) so failures are visible; ensure the retry runs as a goroutine
and use the existing logger and metrics client to report attempts, backoff,
final failure (if you cap retries), and eventual success.
In `@services/order/evm.go`:
- Around line 113-116: The calls to createOrderFor7702, refundOrderFor7702, and
settleOrderFor7702 currently discard their returned map (which contains
transactionHash and blockNumber) — capture and persist those on the order record
for observability/idempotency instead of using "_, err = ..."; modify the call
sites that call createOrderFor7702, refundOrderFor7702, and settleOrderFor7702
to assign the returned map (e.g., txInfo, err := ...), extract
txInfo["transactionHash"] and txInfo["blockNumber"], and update the order entity
(or call an existing order persistence method) to store those fields (or add
them to the order struct if missing), handling nil/absent keys and errors before
proceeding.
- Around line 221-235: Parse the aggregator ECDSA key once in NewOrderEVM and
store both the parsed private key and derived address on the service struct
(e.g., add fields s.aggregatorKey and s.aggregatorAddr alongside
s.nonceManager), then remove the repeated
crypto.HexToECDSA/crypto.PubkeyToAddress calls from createOrderFor7702,
refundOrderFor7702, and settleOrderFor7702 and use s.aggregatorKey and
s.aggregatorAddr instead; ensure NewOrderEVM returns an error if HexToECDSA
fails so callers handle initialization errors and update any error messages in
the helper functions to reference the stored fields rather than re-parsing.
In `@tasks/refunds.go`:
- Around line 127-129: The refund response from refundExpiredOrderFor7702 is
currently discarded; capture its returned map (transactionHash, blockNumber) and
persist those values onto the order record (e.g., add/assign
RefundTransactionHash and RefundBlockNumber on the order struct if absent) then
call the existing order save/update function (e.g., UpdateOrder, SaveOrder, or
orderRepo.Save) to persist changes; remove the `@todo` comment. Ensure
refundExpiredOrderFor7702(ctx, order, ...) return value is checked for nil and
handle extraction of "transactionHash" and "blockNumber" keys safely before
saving.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ent/migrate/migrations/atlas.sumis excluded by!**/*.sum
📒 Files selected for processing (23)
controllers/index.gocontrollers/sender/sender.goent/migrate/migrations/20260225165800_add_sponsorship_mode_and_wallet_type.sqlent/migrate/schema.goent/mutation.goent/network.goent/network/network.goent/network/where.goent/network_create.goent/network_update.goent/paymentorder.goent/paymentorder/paymentorder.goent/paymentorder/where.goent/paymentorder_create.goent/paymentorder_update.goent/schema/network.goent/schema/paymentorder.gomain.goservices/common/indexer.goservices/common/order.goservices/order/evm.goservices/receive_address.gotasks/refunds.go
ent/migrate/migrations/20260225165800_add_sponsorship_mode_and_wallet_type.sql
Show resolved
Hide resolved
- Added `SetWalletType` method to `PaymentOrderCreate` and relevant test cases to support the new wallet type feature. - Updated `InitiatePaymentOrder` and `InitiatePaymentOrderV2` methods to utilize the request context for creating smart addresses. - Improved error handling in refund processing by validating hex address formats for receive and return addresses. - Adjusted database schema and migration scripts to ensure proper handling of the `wallet_type` field across various components.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
tasks/refunds.go (2)
69-71:⚠️ Potential issue | 🟠 MajorRemove the created-at cutoff from expired refund selection.
Line 70 (
CreatedAtGTE) can cause olderStatusExpiredorders to be skipped forever and never refunded.🐛 Proposed fix
expiredOrders, err := storage.Client.PaymentOrder. Query(). Where( paymentorder.StatusEQ(paymentorder.StatusExpired), - paymentorder.CreatedAtGTE(time.Now().Add(-(RefundsInterval * time.Minute))), ).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tasks/refunds.go` around lines 69 - 71, Remove the CreatedAtGTE cutoff so expired orders aren't permanently skipped: in the payment order query that currently uses paymentorder.StatusEQ(paymentorder.StatusExpired) together with paymentorder.CreatedAtGTE(time.Now().Add(-(RefundsInterval * time.Minute))), delete the paymentorder.CreatedAtGTE(...) filter (leave the StatusEQ and any other filters intact) so all StatusExpired orders are considered for refunds regardless of CreatedAt timestamp.
132-151:⚠️ Potential issue | 🟠 MajorUse an intermediate status for Engine-based refunds.
Line 134 calls the smart-wallet Engine path (fire-and-forget), but Line 150 immediately sets
StatusRefunded. If Engine later fails, retries are blocked.🐛 Proposed fix
- _, err = order.Update().SetStatus(paymentorder.StatusRefunded).Save(ctx) + targetStatus := paymentorder.StatusRefunded + if order.WalletType == paymentorder.WalletTypeSmartWallet { + targetStatus = paymentorder.StatusRefunding + } + _, err = order.Update().SetStatus(targetStatus).Save(ctx)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tasks/refunds.go` around lines 132 - 151, The code immediately marks orders as paymentorder.StatusRefunded after calling the fire-and-forget Engine path refundExpiredOrderForSmartWallet, which blocks retries if the Engine later fails; change the flow to set an intermediate status (e.g., add paymentorder.StatusRefundPending) for paymentorder.WalletTypeSmartWallet after calling refundExpiredOrderForSmartWallet, and only set paymentorder.StatusRefunded after a confirmed successful synchronous refund (keep existing behavior for refundExpiredOrderFor7702). Concretely: introduce the new StatusRefundPending enum, call refundExpiredOrderForSmartWallet and on nil error update the order via order.Update().SetStatus(paymentorder.StatusRefundPending).Save(ctx) (do not set StatusRefunded here), while for paymentorder.WalletTypeEoa7702 continue to set StatusRefunded after successful refund; ensure error handling still returns on failures so retries remain possible.
🧹 Nitpick comments (2)
controllers/sender/sender.go (1)
421-445: Mode-to-wallet/webhook logic is duplicated across v1 and v2.The same branching and webhook post-commit flow appears twice. Extracting shared helpers will reduce drift and make future changes safer.
Also applies to: 560-584, 1060-1084, 1177-1201
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controllers/sender/sender.go` around lines 421 - 445, The code duplicates the mode->walletType and createTransferWebhook logic around the CreateSmartAddress/receiveAddress setup; extract that branching into a small helper (e.g., ResolveWalletTypeAndWebhook(mode string) (paymentorder.WalletType, bool)) and also consider a CreateReceiveAddress helper that wraps receiveAddressService.CreateSmartAddress, assigns receiveAddress/receiveAddressSalt and sets receiveAddressExpiry = time.Now().Add(orderConf.ReceiveAddressValidity). Replace the duplicated blocks that reference token.Edges.Network.SponsorshipMode, CreateSmartAddress, walletType, createTransferWebhook, receiveAddress, receiveAddressSalt and receiveAddressExpiry with calls to these helpers in all locations (the current block and the other occurrences you noted).utils/test/db.go (1)
279-287: Fail fast on invalidwallet_typeoverride types.At Line 281, unsupported
wallet_typetypes are silently ignored and default tosmart_wallet, which can mask broken test setup.💡 Proposed refactor
walletType := paymentorder.WalletTypeSmartWallet if v, ok := payload["wallet_type"]; ok && v != nil { - if s, ok := v.(string); ok { - walletType = paymentorder.WalletType(s) - } else if wt, ok := v.(paymentorder.WalletType); ok { - walletType = wt - } + switch wt := v.(type) { + case string: + walletType = paymentorder.WalletType(wt) + case paymentorder.WalletType: + walletType = wt + default: + return nil, fmt.Errorf("wallet_type must be string or paymentorder.WalletType") + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/test/db.go` around lines 279 - 287, The code currently ignores unsupported types for payload["wallet_type"] and silently uses paymentorder.WalletTypeSmartWallet; change the logic so that after checking string and paymentorder.WalletType cases you explicitly fail fast for any other type by returning an error (or calling the test helper's fatal) that includes the actual received type and key name; update the block around walletType, payload and paymentorder.WalletType(s) to include an else branch that constructs and returns a clear error like "invalid type for wallet_type: got %T" referencing payload and walletType.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controllers/sender/sender_test.go`:
- Around line 233-234: Add a test case in controllers/sender/sender_test.go that
seeds and asserts creation of a self-sponsored order by using
SetWalletType(paymentorder.WalletTypeEoa7702) and the self_sponsored flag
(mirror existing smart-wallet test patterns), asserting the controller's
routing/webhook behavior for the EOA-7702 path; use the helper utilities in
utils/test/db.go to create the order and related fixtures and add assertions
similar to the existing SmartWallet checks to ensure the new mode branch is
covered.
In
`@ent/migrate/migrations/20260225165800_add_sponsorship_mode_and_wallet_type.sql`:
- Around line 2-5: Update the migration to enforce valid enum-like values and
the self-sponsored invariant: replace the free-form varchar usage for
networks.sponsorship_mode and payment_orders.wallet_type with constrained values
(e.g., add CHECK constraints listing allowed strings for sponsorship_mode such
as 'thirdweb' and 'self_sponsored' and for wallet_type such as 'smart_wallet'
and other valid types) and add a CHECK on networks ensuring that if
sponsorship_mode = 'self_sponsored' then delegation_contract_address IS NOT NULL
AND delegation_contract_address <> '' (or equivalent non-empty check); implement
these as ALTER TABLE ... ADD CONSTRAINT ... CHECK(...) statements naming the
constraints clearly so the DB rejects invalid values at write time.
In `@ent/migrate/schema.go`:
- Line 218: The paymentorder schema's wallet_type enum lacks the schema default
that the SQL migration sets; update the field definition in
ent/schema/paymentorder.go by adding .Default("smart_wallet") to the
field.Enum("wallet_type") declaration (the one that currently uses
Values("smart_wallet", "eoa_7702")) so the Ent schema matches the migration,
then run make gen-ent to regenerate the generated code.
In `@utils/eip7702.go`:
- Around line 183-191: The polling loop currently retries on any error from
client.TransactionReceipt (called with ctx and signedTx.Hash()), which masks
real RPC or response errors; modify the loop to import errors and, when
TransactionReceipt returns an error, check if errors.Is(err, ethereum.NotFound)
and only continue retrying for NotFound, otherwise immediately return the error
(e.g., return nil, fmt.Errorf("getting receipt for %s: %w",
signedTx.Hash().Hex(), err)). Keep the existing context cancellation and
time.After(2*time.Second) backoff behavior.
---
Duplicate comments:
In `@tasks/refunds.go`:
- Around line 69-71: Remove the CreatedAtGTE cutoff so expired orders aren't
permanently skipped: in the payment order query that currently uses
paymentorder.StatusEQ(paymentorder.StatusExpired) together with
paymentorder.CreatedAtGTE(time.Now().Add(-(RefundsInterval * time.Minute))),
delete the paymentorder.CreatedAtGTE(...) filter (leave the StatusEQ and any
other filters intact) so all StatusExpired orders are considered for refunds
regardless of CreatedAt timestamp.
- Around line 132-151: The code immediately marks orders as
paymentorder.StatusRefunded after calling the fire-and-forget Engine path
refundExpiredOrderForSmartWallet, which blocks retries if the Engine later
fails; change the flow to set an intermediate status (e.g., add
paymentorder.StatusRefundPending) for paymentorder.WalletTypeSmartWallet after
calling refundExpiredOrderForSmartWallet, and only set
paymentorder.StatusRefunded after a confirmed successful synchronous refund
(keep existing behavior for refundExpiredOrderFor7702). Concretely: introduce
the new StatusRefundPending enum, call refundExpiredOrderForSmartWallet and on
nil error update the order via
order.Update().SetStatus(paymentorder.StatusRefundPending).Save(ctx) (do not set
StatusRefunded here), while for paymentorder.WalletTypeEoa7702 continue to set
StatusRefunded after successful refund; ensure error handling still returns on
failures so retries remain possible.
---
Nitpick comments:
In `@controllers/sender/sender.go`:
- Around line 421-445: The code duplicates the mode->walletType and
createTransferWebhook logic around the CreateSmartAddress/receiveAddress setup;
extract that branching into a small helper (e.g.,
ResolveWalletTypeAndWebhook(mode string) (paymentorder.WalletType, bool)) and
also consider a CreateReceiveAddress helper that wraps
receiveAddressService.CreateSmartAddress, assigns
receiveAddress/receiveAddressSalt and sets receiveAddressExpiry =
time.Now().Add(orderConf.ReceiveAddressValidity). Replace the duplicated blocks
that reference token.Edges.Network.SponsorshipMode, CreateSmartAddress,
walletType, createTransferWebhook, receiveAddress, receiveAddressSalt and
receiveAddressExpiry with calls to these helpers in all locations (the current
block and the other occurrences you noted).
In `@utils/test/db.go`:
- Around line 279-287: The code currently ignores unsupported types for
payload["wallet_type"] and silently uses paymentorder.WalletTypeSmartWallet;
change the logic so that after checking string and paymentorder.WalletType cases
you explicitly fail fast for any other type by returning an error (or calling
the test helper's fatal) that includes the actual received type and key name;
update the block around walletType, payload and paymentorder.WalletType(s) to
include an else branch that constructs and returns a clear error like "invalid
type for wallet_type: got %T" referencing payload and walletType.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
controllers/sender/sender.gocontrollers/sender/sender_test.goent/migrate/migrations/20260225165800_add_sponsorship_mode_and_wallet_type.sqlent/migrate/schema.goent/paymentorder/paymentorder.goent/paymentorder_create.goent/schema/paymentorder.gotasks/refunds.gotasks/setup_test.goutils/eip7702.goutils/test/db.go
🚧 Files skipped from review as they are similar to previous changes (1)
- ent/schema/paymentorder.go
ent/migrate/migrations/20260225165800_add_sponsorship_mode_and_wallet_type.sql
Show resolved
Hide resolved
- Introduced a new SQL migration to enforce a constraint that requires a non-empty `delegation_contract_address` when `sponsorship_mode` is set to 'self_sponsored' in the `networks` table. - Updated the atlas checksum to include the new migration file.
- Updated the `wallet_type` field in the `PaymentOrder` schema to include a default value of "smart_wallet", enhancing the schema's usability and ensuring consistent behavior during order creation. - Improved error handling in the `SendKeeperTx` function by adding a check for non-not-found errors, providing clearer error messages for transaction receipt retrieval failures.
- Modified the logging in the ProcessExpiredOrdersRefunds function to use the order's ReceiveAddress instead of a previously incorrect variable, ensuring accurate error reporting and improved traceability during refund operations.
- Added SetNillableWalletType method to allow setting the "wallet_type" field conditionally based on a pointer value, improving flexibility in order creation. - Updated defaults method to ensure a default wallet type is set if none is provided, ensuring consistent behavior during payment order creation.
…alization - Removed the dependency on EngineService in SenderController, simplifying the instantiation of ReceiveAddressService. - Updated the payment order initiation methods to generate unique labels for smart address creation, enhancing traceability. - Improved error handling in webhook creation, ensuring that unsupported chains do not cause order failures. - Refactored transaction commit logic for better clarity and consistency in error responses during payment order processing.
…d logic - Renamed the `sponsorship_mode` field to `wallet_service` across the codebase, reflecting a shift in terminology and functionality. - Updated enum values from "thirdweb" and "self_sponsored" to "engine" and "native" respectively, ensuring alignment with the new wallet service model. - Adjusted related methods, mutations, and database schema to accommodate the changes, enhancing clarity and consistency in the codebase. - Implemented a migration script to handle the renaming and value updates in the database, ensuring data integrity during the transition.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
4 tasks
- Enhanced sender tests to include a new test case for EIP-7702 authorization, verifying the creation of payment orders on the native network. - Introduced setup logic for native network and token, ensuring proper initialization for EIP-7702 related tests. - Updated refund processing to conditionally parse the aggregator key only when necessary, improving efficiency in handling expired orders. - Improved error handling and assertions in the new test case to ensure accurate validation of the EIP-7702 authorization process.
…rization - Adjusted assertions in the sender tests to reflect the correct totalOrders count after including EIP-7702 authorization. - Updated comments to clarify the expected totalOrders based on the new test scenarios, ensuring accurate validation of order processing.
- Updated the createOrderFor7702 function to use the pending nonce for authorization, ensuring accurate nonce management. - Introduced a new error, ErrTxBroadcastNoReceipt, to handle cases where a transaction is broadcast but the receipt is not available, improving clarity in error messages. - Enhanced the ReleaseNonce method to clarify its behavior under concurrent conditions, ensuring safe nonce management without rolling back the counter. - Improved error handling in SendKeeperTx to provide more informative messages when transaction receipts are not obtained.
- Updated comments in nonce management tests to reflect that ReleaseNonce is a no-op, ensuring the internal counter does not roll back. - Adjusted assertions to verify that the nonce counter remains consistent after release operations, improving test accuracy and clarity.
chibie
requested changes
Feb 27, 2026
Contributor
chibie
left a comment
chibie
left a comment
There was a problem hiding this comment.
also, abstract the 7702 calls into a general util or service for write operations
- Eliminated the wallet_type field from the PaymentOrder schema and associated mutations, streamlining the payment order structure. - Updated related methods and predicates to remove references to wallet_type, ensuring consistency across the codebase. - Adjusted database migrations to reflect the removal of wallet_type, enhancing clarity and reducing complexity in order management. - Introduced new services for EIP-7702 handling, improving the overall architecture for transaction processing.
ent/migrate/migrations/20260225165800_add_sponsorship_mode_and_wallet_type.sql
Outdated
Show resolved
Hide resolved
- Reformatted the `CryptoConfiguration` struct in `crypto.go` for consistent alignment of fields, enhancing readability. - Updated test files in `index_test.go`, `auth_test.go`, and `sender_test.go` to improve code clarity and maintainability by standardizing spacing and indentation. - Removed unnecessary whitespace and ensured consistent formatting across various sections of the codebase, contributing to a cleaner code structure.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Replace Thirdweb Engine's EIP-4337 smart wallet infrastructure with native EIP-7702 delegation for EVM receive addresses. This removes the third-party dependency on Thirdweb, centralizes gas sponsorship through a single keeper wallet (
AggregatorEVMPrivateKey), and uses direct RPC for all on-chain operations.Key changes:
sponsorship_modeenum (thirdweb|self_sponsored) anddelegation_contract_addressonNetworkentity.wallet_typeenum (smart_wallet|eoa_7702) so each order records how it was created.CreateSmartAddress(ctx, label, mode)that branches to Thirdweb or local EOA + encrypted salt; transfer webhooks only for Thirdweb.createOrderForSmartWallet/createOrderFor7702settleOrderForSmartWallet/settleOrderFor7702refundOrderForSmartWallet/refundOrderFor7702(map[string]interface{}, error)with optionaltransactionHashandblockNumber(7702 path) for future use; smart wallet path returns(nil, nil).refundExpiredOrderForSmartWallet(Engine send)refundExpiredOrderFor7702(keeper tx)StatusRefundedafter success.NonceManager(AcquireNonce, ReleaseNonce, SubmitWithNonce) for keeper txs; reverted-but-mined txs do not release nonce.EngineService.GetContractEventsWithFallback(RPC first, Insight fallback).References
Closes #705
Testing
NonceManagercovering concurrent access, nonce-too-low retry, and release logic (utils/nonce_manager_test.go).utils/eip7702_test.go).Testing
go test ./...run; all packages pass except pre-existing failure inrouters/middleware(TestRateLimit), unrelated to this feature.Manual: configure a network with
sponsorship_mode: "self_sponsored"anddelegation_contract_address; create/settle/refund orders and run expired-order refund task for both wallet types.This change adds test coverage for new/changed/fixed functionality
Checklist
mainBy submitting a PR, I agree to Paycrest's Contributor Code of Conduct and Contribution Guide.
Summary by CodeRabbit
New Features
Refactor
Chores