feat: Implement balance checks for L2PS transactions to ensure suffic…#680
Open
feat: Implement balance checks for L2PS transactions to ensure suffic…#680
Conversation
…ient funds before processing
Contributor
ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan Review Summary by QodoImplement balance checks for L2PS and regular transactions WalkthroughsDescription• Add balance validation for L2PS encrypted transactions before mempool insertion • Implement sender balance checks for regular transactions to prevent insufficient funds • Export L2PS_TX_FEE constant for reuse across modules • Add error logging for failed broadcast transactions File Changes1. src/libs/blockchain/routines/validateTransaction.ts
|
|
Contributor
ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan Code Review by Qodo
1. Logging can throw on failure
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+82
to
+84
| if (!result.success) { | ||
| log.error(`[SERVER] broadcastTx FAILED — returning to client: result=${returnValue.result}, extra=${JSON.stringify(returnValue.extra)}, response.extra=${result.response?.extra}`) | ||
| } |
Contributor
There was a problem hiding this comment.
1. Logging can throw on failure 🐞 Bug ⛯ Reliability
In broadcastTx failure handling, manageExecution JSON.stringifies returnValue.extra; if extra is a non-serializable object (e.g., contains circular refs or BigInt), the log statement can throw and break the error-handling path (client may not receive the intended error response).
Agent Prompt
### Issue description
`manageExecution` logs `broadcastTx` failures using `JSON.stringify(returnValue.extra)`. Because `extra` can be an object (and potentially non-JSON-serializable), the logging line can throw and break the error path.
### Issue Context
`returnValue.extra` is populated from `result.extra`, and `handleExecuteTransaction` assigns objects to `result.extra` in multiple branches.
### Fix Focus Areas
- src/libs/network/manageExecution.ts[69-85]
### Suggested fix
- Wrap the stringify in a try/catch and fall back to a safe representation.
- Optionally implement a small `safeStringify` helper that:
- converts `bigint` to string
- handles circular references (or falls back to `util.inspect`)
- truncates overly large outputs
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+80
to
+102
| async function checkSenderBalance(decryptedTx: Transaction): Promise<string | null> { | ||
| const sender = decryptedTx.content.from as string | ||
| if (!sender) return "Missing sender address in decrypted transaction" | ||
|
|
||
| // Extract amount from native payload | ||
| let amount = 0 | ||
| if (decryptedTx.content.type === "native" && Array.isArray(decryptedTx.content.data)) { | ||
| const nativePayload = decryptedTx.content.data[1] as INativePayload | ||
| if (nativePayload?.nativeOperation === "send") { | ||
| const [, sendAmount] = nativePayload.args as [string, number] | ||
| amount = sendAmount || 0 | ||
| } | ||
| } | ||
|
|
||
| const totalRequired = amount + L2PS_TX_FEE | ||
| try { | ||
| const balance = await L2PSTransactionExecutor.getBalance(sender) | ||
| if (balance < BigInt(totalRequired)) { | ||
| return `Insufficient balance: need ${totalRequired} (${amount} + ${L2PS_TX_FEE} fee) but have ${balance}` | ||
| } | ||
| } catch (error) { | ||
| return `Balance check failed: ${error instanceof Error ? error.message : "Unknown error"}` | ||
| } |
Contributor
There was a problem hiding this comment.
2. L2ps fee/balance check inconsistent 🐞 Bug ✓ Correctness
The new L2PS balance pre-checks always add L2PS_TX_FEE (even when the executor does not charge any fee for that tx type), and they don’t validate sendAmount like the executor does. This can incorrectly reject L2PS transactions that would otherwise execute successfully (e.g., non-send native ops, or non-native txs with only gcr_edits) and can also produce misleading errors/behavior when the amount is malformed.
Agent Prompt
### Issue description
`checkSenderBalance` / `checkL2PSBalance` currently require `L2PS_TX_FEE` for all decrypted L2PS transactions, but the executor only checks/burns the fee for `nativeOperation === "send"` and may accept other tx types without fees. This mismatch can cause false rejections. Additionally, the pre-check doesn’t validate the extracted `sendAmount` like the executor does.
### Issue Context
- Executor fee+amount checks are implemented in `handleNativeTransaction` for `nativeOperation === "send"`.
- Other tx types (non-native with `gcr_edits`, or unknown native operations) do not burn/check a fee.
### Fix Focus Areas
- src/libs/network/routines/transactions/handleL2PS.ts[80-102]
- src/libs/blockchain/routines/validateTransaction.ts[145-176]
- src/libs/l2ps/L2PSTransactionExecutor.ts[158-220]
### Suggested fix
1. Decide on policy:
- **Option A (match current executor):** Only add `L2PS_TX_FEE` to `totalRequired` for txs where the executor actually burns/charges the fee (currently native `send`).
- **Option B (enforce fee for all L2PS txs):** Update `L2PSTransactionExecutor.generateGCREdits`/handlers to also burn/check the fee for other tx categories, then keep pre-check consistent.
2. Validate amount exactly as executor does before BigInt conversion:
- `typeof sendAmount === 'number' && Number.isFinite(sendAmount) && Number.isInteger(sendAmount) && sendAmount > 0`
3. Compute required balance using BigInt arithmetic:
- `const required = BigInt(sendAmount) + BigInt(L2PS_TX_FEE)`
- Avoid `BigInt(amount + fee)`.
4. Consider extracting a shared helper (e.g., in `L2PSTransactionExecutor`) to compute required balance so pre-check and executor cannot drift.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
1 participant
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.
…ient funds before processing