fix: Bradbury ABI compatibility and testnet RPC#145
Conversation
Add Bradbury testnet as a new network option with v0.6 ConsensusMain and ConsensusData ABIs, reusing the existing Staking ABI. Bradbury is the developer-facing testnet for deploying intelligent contracts with LLM validation.
- decodeTransaction: normalize initialRotations/txCalldata field names from Bradbury ABI (V06) which differ from Asimov's numOfInitialValidators/txData - Update testnet RPC URLs to GenLayer RPC nodes - Remove WebSocket URLs from testnet chains (not available) - Add unit tests for Bradbury field normalization in decodeTransaction - Add smoke tests for transaction decoding, gen_call, and account balance - Switch smoke tests from raw viem clients to genlayer-js createClient to avoid id:0 rejection by GenLayer RPC
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR introduces support for a new testnet ("testnetBradbury") across the genlayer-js package. Changes include exporting a new chain configuration, updating the Network type union, integrating testnetBradbury into wallet connections, normalizing Bradbury-era transaction field names in decoders, updating RPC endpoints for existing testnets, and refactoring tests to support multiple testnets dynamically. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/transactions/decoders.ts (1)
77-80: Normalization logic handles ABI compatibility correctly.The fallback chain (
tx.txData ?? (tx as any).txCalldata) and (tx.numOfInitialValidators ?? (tx as any).initialRotations) properly normalizes Bradbury V06 field names to the canonical Asimov names.The
as anycast is necessary sinceGenLayerRawTransactiondoesn't include Bradbury-era field names. Consider extending the type definition insrc/types/transactions.tsto include optional Bradbury fields (txCalldata?: Hex,initialRotations?: bigint) for better type safety and documentation, though this is not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/transactions/decoders.ts` around lines 77 - 80, The normalization is fine but we should add the Bradbury-era optional fields to the GenLayerRawTransaction type for type safety: update src/types/transactions.ts to extend GenLayerRawTransaction (or the appropriate interface) with optional properties txCalldata?: Hex and initialRotations?: bigint, then remove the need for (tx as any) casts in src/transactions/decoders.ts where txData and numOfInitialValidators are derived so the code uses tx.txData ?? tx.txCalldata and tx.numOfInitialValidators ?? tx.initialRotations with proper typing.tests/transactions.test.ts (1)
4-6: Use path alias@/*for imports fromsrc/directory.The imports should use the configured path alias instead of relative paths.
Suggested fix
-import { decodeTransaction, simplifyTransactionReceipt } from "../src/transactions/decoders"; +import { decodeTransaction, simplifyTransactionReceipt } from "@/transactions/decoders"; import { localnet } from "../src/chains/localnet"; -import type { GenLayerRawTransaction } from "../src/types/transactions"; +import type { GenLayerRawTransaction } from "@/types/transactions";As per coding guidelines: "Use path alias
@/*for imports fromsrc/directory".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/transactions.test.ts` around lines 4 - 6, Replace the relative imports with the project path alias: change imports that currently reference "../src/transactions/decoders", "../src/chains/localnet", and "../src/types/transactions" to use "@/transactions/decoders" for decodeTransaction and simplifyTransactionReceipt, "@/chains/localnet" for localnet, and "@/types/transactions" for GenLayerRawTransaction so the test uses the configured path alias instead of relative paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/chains/testnetAsimov.ts`:
- Around line 5-7: The inline comment "// chains/localnet.ts" at the top of this
file is a stale copy-paste and should be removed or replaced with an accurate
note; locate the TESTNET_JSON_RPC_URL constant and update or delete the
incorrect comment so the header reflects that this is testnetAsimov (or remove
the file-path comment entirely) to avoid confusion.
In `@tests/smoke.test.ts`:
- Around line 163-171: The test labeled "getTransaction should decode without
crashing on a recent finalized tx" does not call getTransaction and only calls
createClient and getBlockNumber; update the test to either rename it to reflect
it's only checking getBlockNumber or implement a real transaction decoding
assertion: use createClient({chain}) to fetch a recent block (e.g.,
client.getBlock or client.getBlockWithTransactions), pick a transaction hash
from the block, call client.getTransaction(txHash) and assert the returned
transaction/decoded fields are defined (or that decoding does not throw),
keeping the same TIMEOUT and test structure; reference createClient,
getBlock/getBlockWithTransactions, getTransaction and the existing test name
when making the change.
---
Nitpick comments:
In `@src/transactions/decoders.ts`:
- Around line 77-80: The normalization is fine but we should add the
Bradbury-era optional fields to the GenLayerRawTransaction type for type safety:
update src/types/transactions.ts to extend GenLayerRawTransaction (or the
appropriate interface) with optional properties txCalldata?: Hex and
initialRotations?: bigint, then remove the need for (tx as any) casts in
src/transactions/decoders.ts where txData and numOfInitialValidators are derived
so the code uses tx.txData ?? tx.txCalldata and tx.numOfInitialValidators ??
tx.initialRotations with proper typing.
In `@tests/transactions.test.ts`:
- Around line 4-6: Replace the relative imports with the project path alias:
change imports that currently reference "../src/transactions/decoders",
"../src/chains/localnet", and "../src/types/transactions" to use
"@/transactions/decoders" for decodeTransaction and simplifyTransactionReceipt,
"@/chains/localnet" for localnet, and "@/types/transactions" for
GenLayerRawTransaction so the test uses the configured path alias instead of
relative paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8f7f6101-ac0a-4a5d-866a-756769a0ce36
📒 Files selected for processing (10)
CLAUDE.mdREADME.mdsrc/chains/index.tssrc/chains/testnetAsimov.tssrc/chains/testnetBradbury.tssrc/transactions/decoders.tssrc/types/network.tssrc/wallet/connect.tstests/smoke.test.tstests/transactions.test.ts
| // chains/localnet.ts | ||
| const TESTNET_JSON_RPC_URL = "https://zksync-os-testnet-genlayer.zksync.dev"; | ||
| const TESTNET_WS_URL = "wss://zksync-os-testnet-genlayer.zksync.dev/ws"; | ||
| const TESTNET_JSON_RPC_URL = "http://34.12.136.220:9151"; | ||
| // WebSocket not available on testnet GenLayer RPC nodes |
There was a problem hiding this comment.
Stale comment: file path reference is incorrect.
Line 5 contains // chains/localnet.ts but this file is testnetAsimov.ts. This appears to be a copy-paste artifact.
✏️ Proposed fix
-// chains/localnet.ts
const TESTNET_JSON_RPC_URL = "http://34.12.136.220:9151";
// WebSocket not available on testnet GenLayer RPC nodes📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // chains/localnet.ts | |
| const TESTNET_JSON_RPC_URL = "https://zksync-os-testnet-genlayer.zksync.dev"; | |
| const TESTNET_WS_URL = "wss://zksync-os-testnet-genlayer.zksync.dev/ws"; | |
| const TESTNET_JSON_RPC_URL = "http://34.12.136.220:9151"; | |
| // WebSocket not available on testnet GenLayer RPC nodes | |
| const TESTNET_JSON_RPC_URL = "http://34.12.136.220:9151"; | |
| // WebSocket not available on testnet GenLayer RPC nodes |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/chains/testnetAsimov.ts` around lines 5 - 7, The inline comment "//
chains/localnet.ts" at the top of this file is a stale copy-paste and should be
removed or replaced with an accurate note; locate the TESTNET_JSON_RPC_URL
constant and update or delete the incorrect comment so the header reflects that
this is testnetAsimov (or remove the file-path comment entirely) to avoid
confusion.
| // ─── Transaction Decoding (getTransaction) ───────────────────────────────── | ||
|
|
||
| describe(`Testnet ${name} - Transaction Decoding`, () => { | ||
| it("getTransaction should decode without crashing on a recent finalized tx", async () => { | ||
| const client = createClient({chain}); | ||
| const blockNumber = await client.getBlockNumber(); | ||
| expect(blockNumber).toBeGreaterThan(0n); | ||
| }, TIMEOUT); | ||
| }); |
There was a problem hiding this comment.
Test does not match description: "Transaction Decoding" only fetches block number.
The test is named "getTransaction should decode without crashing on a recent finalized tx" but only calls getBlockNumber(). It doesn't actually call getTransaction or test any decoding logic.
Either rename this test to reflect what it actually does, or implement actual transaction decoding verification.
💡 Suggested implementation for actual transaction decoding test
describe(`Testnet ${name} - Transaction Decoding`, () => {
- it("getTransaction should decode without crashing on a recent finalized tx", async () => {
+ it("should successfully fetch block number", async () => {
const client = createClient({chain});
const blockNumber = await client.getBlockNumber();
expect(blockNumber).toBeGreaterThan(0n);
}, TIMEOUT);
+
+ // TODO: Add actual transaction decoding test when a known finalized tx hash is available
+ // it("getTransaction should decode without crashing", async () => {
+ // const client = createClient({chain});
+ // const tx = await client.getTransaction({ hash: KNOWN_TX_HASH });
+ // expect(tx).toBeDefined();
+ // expect(tx.txDataDecoded).toBeDefined();
+ // }, TIMEOUT);
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/smoke.test.ts` around lines 163 - 171, The test labeled "getTransaction
should decode without crashing on a recent finalized tx" does not call
getTransaction and only calls createClient and getBlockNumber; update the test
to either rename it to reflect it's only checking getBlockNumber or implement a
real transaction decoding assertion: use createClient({chain}) to fetch a recent
block (e.g., client.getBlock or client.getBlockWithTransactions), pick a
transaction hash from the block, call client.getTransaction(txHash) and assert
the returned transaction/decoded fields are defined (or that decoding does not
throw), keeping the same TIMEOUT and test structure; reference createClient,
getBlock/getBlockWithTransactions, getTransaction and the existing test name
when making the change.
1 participant
Summary
decodeTransactioncrash on Bradbury: normalizeinitialRotations/txCalldatafield names from V06 ABI34.91.102.53:9151, Asimov:34.12.136.220:9151)createClient(avoids viemid:0rejection)Test plan
npm test— 22 unit tests passnpm run test:smoke— 32/32 smoke tests pass on both testnetsSummary by CodeRabbit
New Features
Documentation
Improvements
Tests