Skip to content

Commit 657cd91

Browse files
Antoine de Chevignéclaude
Antoine de Chevigné
and
claude
committed
Address code review suggestions for OP Stack
- Fix race condition in batch index assignment using transaction with lock - Add input validation and sanitization for pagination parameters - Add named constants for EIP-4844 versioned hash calculations - Add 404 handling when OP entities are not found - Add optional chaining for template property access in Vue components 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
1 parent 0cd7aee commit 657cd91

File tree

12 files changed

+114
-43
lines changed

12 files changed

+114
-43
lines changed

‎run/api/opBatches.js‎

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,16 @@ const express = require('express');
22
const router = express.Router();
33
const workspaceAuthMiddleware = require('../middlewares/workspaceAuth');
44
const db = require('../lib/firebase');
5-
const { unmanagedError } = require('../lib/errors');
5+
const { unmanagedError, managedError } = require('../lib/errors');
6+
7+
/**
8+
* Sanitize pagination parameters
9+
*/
10+
const sanitizePagination = (page, itemsPerPage, order) => ({
11+
page: Math.max(1, parseInt(page) || 1),
12+
itemsPerPage: Math.min(100, Math.max(1, parseInt(itemsPerPage) || 10)),
13+
order: ['ASC', 'DESC'].includes(order?.toUpperCase()) ? order.toUpperCase() : 'DESC'
14+
});
615

716
/**
817
* Get paginated list of OP batches for a workspace
@@ -15,7 +24,7 @@ router.get('/', workspaceAuthMiddleware, async (req, res, next) => {
1524
const data = { ...req.query, ...req.params };
1625

1726
try {
18-
const { page, itemsPerPage, order } = data;
27+
const { page, itemsPerPage, order } = sanitizePagination(data.page, data.itemsPerPage, data.order);
1928
const { rows: items, count: total } = await db.getWorkspaceOpBatches(data.workspace.id, page, itemsPerPage, order);
2029

2130
res.status(200).json({ items, total });
@@ -35,6 +44,10 @@ router.get('/:batchIndex', workspaceAuthMiddleware, async (req, res, next) => {
3544
try {
3645
const batch = await db.getOpBatch(data.workspace.id, data.batchIndex);
3746

47+
if (!batch) {
48+
return res.status(404).json({ error: 'Batch not found' });
49+
}
50+
3851
res.status(200).json(batch);
3952
} catch (error) {
4053
unmanagedError(error, req, next);
@@ -53,7 +66,8 @@ router.get('/:batchIndex/transactions', workspaceAuthMiddleware, async (req, res
5366
const data = { ...req.query, ...req.params };
5467

5568
try {
56-
const { total, items } = await db.getOpBatchTransactions(data.workspace.id, data.batchIndex, data.page, data.itemsPerPage, data.order);
69+
const { page, itemsPerPage, order } = sanitizePagination(data.page, data.itemsPerPage, data.order);
70+
const { total, items } = await db.getOpBatchTransactions(data.workspace.id, data.batchIndex, page, itemsPerPage, order);
5771

5872
res.status(200).json({ total, items });
5973
} catch (error) {

‎run/api/opDeposits.js‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,16 @@ const express = require('express');
22
const router = express.Router();
33
const workspaceAuthMiddleware = require('../middlewares/workspaceAuth');
44
const db = require('../lib/firebase');
5-
const { unmanagedError } = require('../lib/errors');
5+
const { unmanagedError, managedError } = require('../lib/errors');
6+
7+
/**
8+
* Sanitize pagination parameters
9+
*/
10+
const sanitizePagination = (page, itemsPerPage, order) => ({
11+
page: Math.max(1, parseInt(page) || 1),
12+
itemsPerPage: Math.min(100, Math.max(1, parseInt(itemsPerPage) || 10)),
13+
order: ['ASC', 'DESC'].includes(order?.toUpperCase()) ? order.toUpperCase() : 'DESC'
14+
});
615

716
/**
817
* Get paginated list of OP deposits for a workspace
@@ -15,7 +24,7 @@ router.get('/', workspaceAuthMiddleware, async (req, res, next) => {
1524
const data = { ...req.query, ...req.params };
1625

1726
try {
18-
const { page, itemsPerPage, order } = data;
27+
const { page, itemsPerPage, order } = sanitizePagination(data.page, data.itemsPerPage, data.order);
1928
const { rows: items, count: total } = await db.getWorkspaceOpDeposits(data.workspace.id, page, itemsPerPage, order);
2029

2130
res.status(200).json({ items, total });
@@ -35,6 +44,10 @@ router.get('/:hash', workspaceAuthMiddleware, async (req, res, next) => {
3544
try {
3645
const deposit = await db.getOpDepositByL1Hash(data.workspace.id, data.hash);
3746

47+
if (!deposit) {
48+
return res.status(404).json({ error: 'Deposit not found' });
49+
}
50+
3851
res.status(200).json(deposit);
3952
} catch (error) {
4053
unmanagedError(error, req, next);

‎run/api/opOutputs.js‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,16 @@ const express = require('express');
22
const router = express.Router();
33
const workspaceAuthMiddleware = require('../middlewares/workspaceAuth');
44
const db = require('../lib/firebase');
5-
const { unmanagedError } = require('../lib/errors');
5+
const { unmanagedError, managedError } = require('../lib/errors');
6+
7+
/**
8+
* Sanitize pagination parameters
9+
*/
10+
const sanitizePagination = (page, itemsPerPage, order) => ({
11+
page: Math.max(1, parseInt(page) || 1),
12+
itemsPerPage: Math.min(100, Math.max(1, parseInt(itemsPerPage) || 10)),
13+
order: ['ASC', 'DESC'].includes(order?.toUpperCase()) ? order.toUpperCase() : 'DESC'
14+
});
615

716
/**
817
* Get paginated list of OP outputs for a workspace
@@ -15,7 +24,7 @@ router.get('/', workspaceAuthMiddleware, async (req, res, next) => {
1524
const data = { ...req.query, ...req.params };
1625

1726
try {
18-
const { page, itemsPerPage, order } = data;
27+
const { page, itemsPerPage, order } = sanitizePagination(data.page, data.itemsPerPage, data.order);
1928
const { rows: items, count: total } = await db.getWorkspaceOpOutputs(data.workspace.id, page, itemsPerPage, order);
2029

2130
res.status(200).json({ items, total });
@@ -35,6 +44,10 @@ router.get('/:outputIndex', workspaceAuthMiddleware, async (req, res, next) => {
3544
try {
3645
const output = await db.getOpOutput(data.workspace.id, data.outputIndex);
3746

47+
if (!output) {
48+
return res.status(404).json({ error: 'Output not found' });
49+
}
50+
3851
res.status(200).json(output);
3952
} catch (error) {
4053
unmanagedError(error, req, next);

‎run/api/opWithdrawals.js‎

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,16 @@ const express = require('express');
22
const router = express.Router();
33
const workspaceAuthMiddleware = require('../middlewares/workspaceAuth');
44
const db = require('../lib/firebase');
5-
const { unmanagedError } = require('../lib/errors');
5+
const { unmanagedError, managedError } = require('../lib/errors');
6+
7+
/**
8+
* Sanitize pagination parameters
9+
*/
10+
const sanitizePagination = (page, itemsPerPage, order) => ({
11+
page: Math.max(1, parseInt(page) || 1),
12+
itemsPerPage: Math.min(100, Math.max(1, parseInt(itemsPerPage) || 10)),
13+
order: ['ASC', 'DESC'].includes(order?.toUpperCase()) ? order.toUpperCase() : 'DESC'
14+
});
615

716
/**
817
* Get paginated list of OP withdrawals for a workspace
@@ -15,7 +24,7 @@ router.get('/', workspaceAuthMiddleware, async (req, res, next) => {
1524
const data = { ...req.query, ...req.params };
1625

1726
try {
18-
const { page, itemsPerPage, order } = data;
27+
const { page, itemsPerPage, order } = sanitizePagination(data.page, data.itemsPerPage, data.order);
1928
const { rows: items, count: total } = await db.getWorkspaceOpWithdrawals(data.workspace.id, page, itemsPerPage, order);
2029

2130
res.status(200).json({ items, total });
@@ -35,6 +44,10 @@ router.get('/:hash', workspaceAuthMiddleware, async (req, res, next) => {
3544
try {
3645
const withdrawal = await db.getOpWithdrawalByL2Hash(data.workspace.id, data.hash);
3746

47+
if (!withdrawal) {
48+
return res.status(404).json({ error: 'Withdrawal not found' });
49+
}
50+
3851
res.status(200).json(withdrawal);
3952
} catch (error) {
4053
unmanagedError(error, req, next);
@@ -52,6 +65,10 @@ router.get('/:hash/proof', workspaceAuthMiddleware, async (req, res, next) => {
5265
try {
5366
const proof = await db.getOpWithdrawalProof(data.workspace.id, data.hash);
5467

68+
if (!proof) {
69+
return res.status(404).json({ error: 'Withdrawal proof not found' });
70+
}
71+
5572
res.status(200).json(proof);
5673
} catch (error) {
5774
unmanagedError(error, req, next);

‎run/jobs/blockSync.js‎

Lines changed: 29 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
const { ProviderConnector } = require('../lib/rpc');
2-
const { Workspace, Explorer, StripeSubscription, RpcHealthCheck, IntegrityCheck, Block, OrbitChainConfig, OpChainConfig, OpBatch } = require('../models');
2+
const { Workspace, Explorer, StripeSubscription, RpcHealthCheck, IntegrityCheck, Block, OrbitChainConfig, OpChainConfig, OpBatch, sequelize } = require('../models');
33
const db = require('../lib/firebase');
44
const logger = require('../lib/logger');
55
const { processRawRpcObject } = require('../lib/utils');
@@ -194,34 +194,38 @@ module.exports = async job => {
194194
});
195195

196196
if (batchInfo) {
197-
// Get the next batch index
198-
const lastBatch = await OpBatch.findOne({
199-
where: { workspaceId: opConfig.workspaceId },
200-
order: [['batchIndex', 'DESC']]
201-
});
202-
const nextBatchIndex = lastBatch ? lastBatch.batchIndex + 1 : 0;
203-
204197
// Find the L1 transaction record if it exists
205198
const l1Transaction = syncedBlock.transactions.find(t => t.hash === tx.hash);
206199

207-
await OpBatch.create({
208-
workspaceId: opConfig.workspaceId,
209-
batchIndex: nextBatchIndex,
210-
l1BlockNumber: batchInfo.l1BlockNumber,
211-
l1TransactionHash: batchInfo.l1TransactionHash,
212-
l1TransactionId: l1Transaction ? l1Transaction.id : null,
213-
l1TransactionIndex: batchInfo.l1TransactionIndex,
214-
epochNumber: batchInfo.l1BlockNumber, // Epoch is typically the L1 block
215-
timestamp: tx.timestamp ? new Date(tx.timestamp * 1000) : new Date(),
216-
txCount: batchInfo.estimatedBlockCount || null,
217-
l2BlockStart: batchInfo.l2BlockStart,
218-
l2BlockEnd: batchInfo.l2BlockEnd,
219-
blobHash: batchInfo.blobHash,
220-
blobData: batchInfo.blobData,
221-
status: 'pending'
200+
// Use transaction with lock to prevent race condition on batch index
201+
await sequelize.transaction(async (t) => {
202+
const lastBatch = await OpBatch.findOne({
203+
where: { workspaceId: opConfig.workspaceId },
204+
order: [['batchIndex', 'DESC']],
205+
lock: t.LOCK.UPDATE,
206+
transaction: t
207+
});
208+
const nextBatchIndex = lastBatch ? lastBatch.batchIndex + 1 : 0;
209+
210+
await OpBatch.create({
211+
workspaceId: opConfig.workspaceId,
212+
batchIndex: nextBatchIndex,
213+
l1BlockNumber: batchInfo.l1BlockNumber,
214+
l1TransactionHash: batchInfo.l1TransactionHash,
215+
l1TransactionId: l1Transaction ? l1Transaction.id : null,
216+
l1TransactionIndex: batchInfo.l1TransactionIndex,
217+
epochNumber: batchInfo.l1BlockNumber, // Epoch is typically the L1 block
218+
timestamp: tx.timestamp ? new Date(tx.timestamp * 1000) : new Date(),
219+
txCount: batchInfo.estimatedBlockCount || null,
220+
l2BlockStart: batchInfo.l2BlockStart,
221+
l2BlockEnd: batchInfo.l2BlockEnd,
222+
blobHash: batchInfo.blobHash,
223+
blobData: batchInfo.blobData,
224+
status: 'pending'
225+
}, { transaction: t });
226+
227+
logger.info(`Created OP batch ${nextBatchIndex} for L2 workspace ${opConfig.workspaceId} from L1 tx ${tx.hash}`);
222228
});
223-
224-
logger.info(`Created OP batch ${nextBatchIndex} for L2 workspace ${opConfig.workspaceId} from L1 tx ${tx.hash}`);
225229
}
226230
} catch (error) {
227231
logger.error(`Error processing OP batch for tx ${tx.hash}: ${error.message}`, { location: 'jobs.blockSync.opBatch', error });

‎run/lib/opBatches.js‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,13 @@ const FRAME_HEADER_SIZE = 23;
1919
// Derivation version byte
2020
const DERIVATION_VERSION_0 = 0;
2121

22+
// EIP-4844 Blob versioned hash constants
23+
// See: https://eips.ethereum.org/EIPS/eip-4844#helpers
24+
const VERSIONED_HASH_VERSION_KZG = 0x01; // Version byte for KZG commitments
25+
const SHA256_HASH_LENGTH_HEX = 64; // SHA-256 produces 32 bytes = 64 hex chars
26+
const VERSIONED_HASH_LENGTH_BYTES = 32; // 1 version byte + 31 hash bytes
27+
const VERSIONED_HASH_HASH_BYTES = 31; // Bytes of hash to include (31 of 32)
28+
2229
/**
2330
* Check if a transaction is a batch submission to the BatchInbox
2431
* @param {Object} tx - Transaction object
@@ -139,10 +146,13 @@ const computeBlobVersionedHash = (kzgCommitment) => {
139146
const commitment = kzgCommitment.startsWith('0x') ? kzgCommitment : '0x' + kzgCommitment;
140147
// EIP-4844 uses SHA-256 for versioned hashes, not keccak256
141148
const hash = ethers.utils.sha256(commitment);
142-
// Remove 0x prefix if present, then prepend version byte 0x01
149+
// Remove 0x prefix if present
143150
const hashWithoutPrefix = hash.startsWith('0x') ? hash.slice(2) : hash;
144-
// Version 0x01 for KZG commitments, result is 0x01 + 31 bytes of hash = 66 chars total
145-
return '0x01' + hashWithoutPrefix.slice(0, 62);
151+
// Versioned hash = version byte + first 31 bytes of SHA-256 hash
152+
// Result: 0x + version (2 hex) + 31 bytes (62 hex) = 66 chars total
153+
const versionHex = VERSIONED_HASH_VERSION_KZG.toString(16).padStart(2, '0');
154+
const truncatedHash = hashWithoutPrefix.slice(0, VERSIONED_HASH_HASH_BYTES * 2);
155+
return '0x' + versionHex + truncatedHash;
146156
};
147157

148158
/**

‎run/tests/api/opBatches.test.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ describe('OpBatches API', () => {
6262
total: 50
6363
});
6464
expect(db.getWorkspaceOpBatches).toHaveBeenCalledWith(
65-
1, '1', '10', 'DESC'
65+
1, 1, 10, 'DESC'
6666
);
6767
done();
6868
})

‎src/components/OpBatches.vue‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@
5858

5959
<template v-slot:item.l1TransactionHash="{ item }">
6060
<span class="text-truncate" style="max-width: 120px; display: inline-block;">
61-
{{ item.l1TransactionHash.slice(0, 10) }}...{{ item.l1TransactionHash.slice(-6) }}
61+
{{ item.l1TransactionHash ? `${item.l1TransactionHash.slice(0, 10)}...${item.l1TransactionHash.slice(-6)}` : '-' }}
6262
</span>
6363
</template>
6464

‎src/components/OpDeposits.vue‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@
5151

5252
<template v-slot:item.l1TransactionHash="{ item }">
5353
<span class="text-truncate" style="max-width: 120px; display: inline-block;">
54-
{{ item.l1TransactionHash.slice(0, 10) }}...{{ item.l1TransactionHash.slice(-6) }}
54+
{{ item.l1TransactionHash ? `${item.l1TransactionHash.slice(0, 10)}...${item.l1TransactionHash.slice(-6)}` : '-' }}
5555
</span>
5656
</template>
5757

‎src/components/OpOutputDetail.vue‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@
4242
<tr>
4343
<td class="font-weight-medium">L1 Transaction</td>
4444
<td style="font-family: monospace;">
45-
{{ output.l1TransactionHash.slice(0, 20) }}...{{ output.l1TransactionHash.slice(-16) }}
45+
{{ output.l1TransactionHash ? `${output.l1TransactionHash.slice(0, 20)}...${output.l1TransactionHash.slice(-16)}` : '-' }}
4646
</td>
4747
</tr>
4848
<tr>

0 commit comments

Comments
 (0)