Merge the 2 Cadence transactions files into a single one - #897
Conversation
WalkthroughConsolidates transaction execution into run.cdc by accepting an array of hex-encoded transactions. Removes batch_run.cdc and updates Go pools to pass array args and reference run.cdc. Adjusts embedding to drop batch_run.cdc. BatchTxPool now invokes run.cdc; SingleTxPool wraps single tx in a Cadence array. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Requester (Go)
participant Cadence run.cdc
participant EVM
Client->>Requester (Go): Submit tx(s)
Note over Requester (Go): Build Cadence tx<br/>args: [hexEncodedTxs], coinbase
Requester (Go)->>Cadence run.cdc: execute([hex], coinbase)
alt exactly one tx
Cadence run.cdc->>EVM: run(tx, coinbase)
EVM-->>Cadence run.cdc: txResult
Cadence run.cdc-->>Requester (Go): assert on status, return
else multiple txs
Cadence run.cdc->>EVM: batchRun(txs, coinbase)
EVM-->>Cadence run.cdc: txResults[]
alt any result is failed or successful
Cadence run.cdc-->>Requester (Go): return
else all invalid
Cadence run.cdc-->>Requester (Go): assert using first error
end
end
Requester (Go)-->>Client: submission result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
f54a93b to
3ebc0eb
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
services/requester/cadence/run.cdc (1)
3-47: Unified single/batch path LGTM; add a guard for empty input (optional)Logic mirrors previous semantics: single uses EVM.run with assert; batch uses EVM.batchRun and only fails when all are invalid. Consider guarding against accidental empty input to avoid a no-op submit.
Apply at the start of execute:
transaction(hexEncodedTxs: [String], coinbase: String) { execute { + // Guard against accidental empty submissions + assert(hexEncodedTxs.length > 0, message: "no_evm_txs_provided")Optionally, avoid the second pass when all invalid by asserting only on the first item (same behavior, less work):
- for txResult in txResults { - assert( - txResult.status == EVM.Status.failed || txResult.status == EVM.Status.successful, - message: "evm_error=\(txResult.errorMessage);evm_error_code=\(txResult.errorCode)" - ) - } + let first = txResults[0] + assert( + first.status == EVM.Status.failed || first.status == EVM.Status.successful, + message: "evm_error=\(first.errorMessage);evm_error_code=\(first.errorCode)" + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
services/requester/batch_tx_pool.go(2 hunks)services/requester/cadence/batch_run.cdc(0 hunks)services/requester/cadence/run.cdc(1 hunks)services/requester/requester.go(0 hunks)services/requester/single_tx_pool.go(1 hunks)
💤 Files with no reviewable changes (2)
- services/requester/cadence/batch_run.cdc
- services/requester/requester.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (3)
services/requester/batch_tx_pool.go (2)
221-228: Switched to runTxScript for batch path — aligned with merged Cadence entrypointUsing replaceAddresses(runTxScript, ...) is correct given batch_run.cdc removal and the new unified transaction.
257-264: Wrap single tx in an array for submitSingleTransaction — correct for new signaturePassing cadence.NewArray([]cadence.Value{hexEncodedTx}) ensures run.cdc takes the expected [String].
services/requester/single_tx_pool.go (1)
101-108: Arg shape update to array is correct
Ensure no leftoverbatch_run.cdcorbatchRunTxScriptreferences remain and that allrunTxScriptinvocations usecadence.NewArray.
janezpodhostnik
left a comment
janezpodhostnik
left a comment
There was a problem hiding this comment.
Nice simplification
3ebc0eb to
070cab1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
services/requester/cadence/run.cdc (1)
5-8: Consider using map for cleaner array transformation.The manual loop works correctly, but Cadence's functional approach would be more idiomatic and concise.
Apply this diff to use
map:- let txs: [[UInt8]] = [] - for tx in hexEncodedTxs { - txs.append(tx.decodeHex()) - } + let txs: [[UInt8]] = hexEncodedTxs.map(fun (tx: String): [UInt8] { + return tx.decodeHex() + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
services/requester/batch_tx_pool.go(2 hunks)services/requester/cadence/batch_run.cdc(0 hunks)services/requester/cadence/run.cdc(1 hunks)services/requester/requester.go(0 hunks)services/requester/single_tx_pool.go(1 hunks)
💤 Files with no reviewable changes (2)
- services/requester/cadence/batch_run.cdc
- services/requester/requester.go
🚧 Files skipped from review as they are similar to previous changes (1)
- services/requester/batch_tx_pool.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (4)
services/requester/single_tx_pool.go (1)
106-106: LGTM! Single transaction correctly wrapped in array.The change correctly wraps the single transaction in a Cadence array to align with the updated
run.cdcsignature that now accepts[String]instead ofString.services/requester/cadence/run.cdc (3)
3-3: LGTM! Signature updated to accept transaction array.The parameter change from
hexEncodedTx: StringtohexEncodedTxs: [String]successfully consolidates the single and batch transaction logic into one file, addressing the PR objective of eliminating duplicate files.
12-22: Verify necessity of separate single-transaction path.The single-transaction path calls
EVM.run, while the batch path callsEVM.batchRun. If both produce identical results for single transactions, this branching adds unnecessary complexity.Please confirm whether:
EVM.runandEVM.batchRun(txs: [singleTx], ...)have different gas costs or behavior- This optimization is measurable and justified
If the behaviors are identical, consider simplifying by always using
EVM.batchRun, which would eliminate lines 12-22 and reduce code paths.
24-47: Verify handling of empty transaction array. IfhexEncodedTxscan be empty,EVM.batchRunreturns an emptytxResults, both loops are skipped, and the Cadence transaction succeeds silently. Ensure Go callers never pass an empty slice or add an explicit Cadence check, for example:assert(txResults.length > 0, message: "no transactions provided")
2 participants
Description
The 2 Cadence transactions:
run.cdc&batch_run.cdccan be merged into a single Cadence transaction, to avoid having to update 2 separate files, when changing the logic.For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores