Conversation
Bumps [hardhat](https://github.com/NomicFoundation/hardhat/tree/HEAD/v-next/hardhat) from 2.28.6 to 3.1.12. - [Release notes](https://github.com/NomicFoundation/hardhat/releases) - [Changelog](https://github.com/NomicFoundation/hardhat/blob/main/v-next/hardhat/CHANGELOG.md) - [Commits](https://github.com/NomicFoundation/hardhat/commits/hardhat@3.1.12/v-next/hardhat) --- updated-dependencies: - dependency-name: hardhat dependency-version: 3.1.12 dependency-type: direct:development update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
…n/docgen/dev/hardhat-3.1.12 chore(deps-dev): bump hardhat from 2.28.6 to 3.1.12 in /docgen
…npm_and_yarn/docgen/dev/hardhat-3.1.12 Revert "chore(deps-dev): bump hardhat from 2.28.6 to 3.1.12 in /docgen"
This commit updates the deployed contract addresses in the README file to reflect the latest deployments on the Sepolia network. Additionally, it enhances the migration script for the AccountBlox contract by improving error handling during initialization and ensuring network information is saved only after successful initialization. These changes improve clarity in documentation and robustness in deployment processes.
fix: update deployed contract addresses in README and migration scripts
Contributor
|
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: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis pull request updates deployed contract addresses in the README for Sepolia, enhances error handling and nonce synchronization in the migration script, adds transaction receipt status validation in the deployment script, and extends test helpers with new methods for batch operations, permission verification, and improved error handling across guard controller and RBAC test infrastructure. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
scripts/sanity-sdk/guard-controller/base-test.ts (1)
830-836: Prefer typed permission records overany-driven decoding.The new permission parsing relies on
anyplus positional fallbacks, which removes compile-time safety for contract response shapes.As per coding guidelines,
**/*.{ts,tsx}should “Expose a type-safe TypeScript SDK (interfaces and classes) for contract interaction … handle async contract calls using typed methods, returning typed results”.Also applies to: 842-860
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/sanity-sdk/guard-controller/base-test.ts` around lines 830 - 836, The code currently decodes permissions with an untyped any and positional fallbacks; replace this with a typed shape and a type-guarded parse: define interfaces (e.g., PermissionRecord, FunctionPermission, FunctionPermissionReturn) that reflect the expected contract response, change the local variable from any[] to Array<FunctionPermission | FunctionPermissionReturn> (or PermissionRecord[]), and implement a small type predicate (e.g., isFunctionPermissions(obj): obj is FunctionPermission[]) to choose the correct field from the result of runtimeRBAC.getActiveRolePermissions(roleHash); update the extraction logic in the permissions → list assignment to use these types and guards (also apply the same typed decoding to the similar code at 842-860) so you retain compile-time safety instead of using positional any fallbacks.migrations/2_deploy_guardian_contracts.cjs (1)
93-96: Nonce sync wait is currently a no-op.Line 95 waits for the same pending nonce already read on Line 93, so the first poll typically succeeds immediately and doesn’t actually synchronize anything.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@migrations/2_deploy_guardian_contracts.cjs` around lines 93 - 96, The code reads currentNonce via web3.eth.getTransactionCount(accounts[0], 'pending') then calls waitForNonceSync(web3, accounts[0], currentNonce), which immediately succeeds because it polls for the same pending nonce; change the logic so waitForNonceSync waits for the nonce to advance (e.g., pass currentNonce + 1 or update waitForNonceSync to expect a target nonce > currentNonce) and have it poll web3.eth.getTransactionCount(..., 'pending') until the pending nonce is greater than currentNonce before proceeding.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@migrations/2_deploy_guardian_contracts.cjs`:
- Around line 127-133: The migration treats a non-zero accountBlox.owner() as
sufficient evidence of full initialization; instead, update the check to verify
all essential initializer state (e.g., broadcaster(), recovery(), timelock() or
an isInitialized() helper) before setting initializedOnChain true. Specifically,
augment the owner check in the block that references accountBlox.owner() and the
similar block later (lines around the other owner() use) to also call and
validate broadcaster(), recovery(), and timelock (or an explicit isInitialized()
if available), ensure none are the zero address / default values, and only then
set initializedOnChain = true; if any required field is missing, log which field
failed and treat the contract as uninitialized.
In `@scripts/sanity-sdk/guard-controller/base-test.ts`:
- Around line 965-971: The resolveTxIdForControllerOperation method currently
returns null when extractTxIdFromReceipt yields no TransactionEvent, which
allows downstream assertions to skip tx-status validation; change this behavior
so that when fromEvent is null the method throws a descriptive error (or
otherwise fails the test) instead of returning null, referencing
resolveTxIdForControllerOperation and extractTxIdFromReceipt so callers cannot
bypass tx-record checks — update any identical logic at the other occurrence
(around lines 751-755) as well.
In `@scripts/sanity-sdk/guard-controller/erc20-mint-controller-tests.ts`:
- Around line 738-751: The tuple fallbacks for extracting
requester/target/executionSelector from getTransactionHistory are using the
wrong index order and the selector-only match can pick a stale tx; update the
fallbacks to match the actual tuple layout returned by getTransactionHistory
(use params.requesterReturn ?? params[0] for requester, params.targetReturn ??
params[1] for target, params.executionSelectorReturn ?? params[6] for execution
selector) and tighten the match before inferring txId by also comparing the mint
amount/nonce or another unique field from the mint request (e.g., params.amount
?? params[3]) and/or tx.timestamp/txId/status so you only accept the exact mint
record that matches ERC20_MINT_SELECTOR and the request payload in
getTransactionHistory/guardController entries.
- Around line 833-842: The current check treats any error message matching
/Missing or invalid parameters/i as an environment limitation; instead, only
downgrade to a skipped test when the RPC error indicates the specific
invalid-params code (e.g., rpc error code -32602) or equivalent context. In the
block around the variables combined, this.mintFlowSkipped and skipTest, inspect
the original RPC error object (not just the combined string) and require
rpcError.code === -32602 (or combined includes '"code":-32602') before setting
mintFlowSkipped and calling skipTest; if the code is absent, preserve the
failure path and surface the error. Ensure logging still prints the full error
and context when you do skip so debugging info is retained.
---
Nitpick comments:
In `@migrations/2_deploy_guardian_contracts.cjs`:
- Around line 93-96: The code reads currentNonce via
web3.eth.getTransactionCount(accounts[0], 'pending') then calls
waitForNonceSync(web3, accounts[0], currentNonce), which immediately succeeds
because it polls for the same pending nonce; change the logic so
waitForNonceSync waits for the nonce to advance (e.g., pass currentNonce + 1 or
update waitForNonceSync to expect a target nonce > currentNonce) and have it
poll web3.eth.getTransactionCount(..., 'pending') until the pending nonce is
greater than currentNonce before proceeding.
In `@scripts/sanity-sdk/guard-controller/base-test.ts`:
- Around line 830-836: The code currently decodes permissions with an untyped
any and positional fallbacks; replace this with a typed shape and a type-guarded
parse: define interfaces (e.g., PermissionRecord, FunctionPermission,
FunctionPermissionReturn) that reflect the expected contract response, change
the local variable from any[] to Array<FunctionPermission |
FunctionPermissionReturn> (or PermissionRecord[]), and implement a small type
predicate (e.g., isFunctionPermissions(obj): obj is FunctionPermission[]) to
choose the correct field from the result of
runtimeRBAC.getActiveRolePermissions(roleHash); update the extraction logic in
the permissions → list assignment to use these types and guards (also apply the
same typed decoding to the similar code at 842-860) so you retain compile-time
safety instead of using positional any fallbacks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 00fefeab-fdc5-4561-9695-84c5e4fea65e
📒 Files selected for processing (6)
README.mdmigrations/2_deploy_guardian_contracts.cjsscripts/deployment/deploy-foundation-libraries.jsscripts/sanity-sdk/guard-controller/base-test.tsscripts/sanity-sdk/guard-controller/erc20-mint-controller-tests.tsscripts/sanity-sdk/runtime-rbac/base-test.ts
Comment on lines
+127
to
+133
| const currentOwner = await accountBlox.owner(); | ||
| if (currentOwner && currentOwner !== "0x0000000000000000000000000000000000000000") { | ||
| initializedOnChain = true; | ||
| console.log("⚠️ initialize() reported an error, but owner() is set on-chain:", currentOwner); | ||
| console.log(" Treating AccountBlox as initialized based on contract state."); | ||
| } else { | ||
| console.log(" owner() check indicates AccountBlox is not initialized (owner is zero address)."); |
Contributor
There was a problem hiding this comment.
owner() alone is not a safe success signal for initialization.
Treating non-zero owner() as success can persist network metadata even when other initializer state is still wrong/missing (e.g., broadcaster/recovery/timelock), leading to a partially usable deployment being marked healthy.
💡 Suggested hardening
- const currentOwner = await accountBlox.owner();
- if (currentOwner && currentOwner !== "0x0000000000000000000000000000000000000000") {
+ const zero = "0x0000000000000000000000000000000000000000";
+ const currentOwner = await accountBlox.owner();
+ const broadcasters = await accountBlox.getBroadcasters();
+ const recovery = await accountBlox.getRecovery();
+ const timelock = await accountBlox.getTimeLockPeriodSec();
+ if (
+ currentOwner && currentOwner !== zero &&
+ Array.isArray(broadcasters) && broadcasters.length > 0 &&
+ recovery && recovery !== zero &&
+ Number(timelock) > 0
+ ) {
initializedOnChain = true;
console.log("⚠️ initialize() reported an error, but owner() is set on-chain:", currentOwner);
console.log(" Treating AccountBlox as initialized based on contract state.");
} else {
- console.log(" owner() check indicates AccountBlox is not initialized (owner is zero address).");
+ console.log(" On-chain checks indicate AccountBlox is not fully initialized.");
}Also applies to: 139-147
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@migrations/2_deploy_guardian_contracts.cjs` around lines 127 - 133, The
migration treats a non-zero accountBlox.owner() as sufficient evidence of full
initialization; instead, update the check to verify all essential initializer
state (e.g., broadcaster(), recovery(), timelock() or an isInitialized() helper)
before setting initializedOnChain true. Specifically, augment the owner check in
the block that references accountBlox.owner() and the similar block later (lines
around the other owner() use) to also call and validate broadcaster(),
recovery(), and timelock (or an explicit isInitialized() if available), ensure
none are the zero address / default values, and only then set initializedOnChain
= true; if any required field is missing, log which field failed and treat the
contract as uninitialized.
Comment on lines
+965
to
+971
| protected async resolveTxIdForControllerOperation(receipt: any): Promise<bigint | null> { | ||
| const fromEvent = this.extractTxIdFromReceipt(receipt); | ||
| if (fromEvent == null) { | ||
| console.log(' ⚠️ No TransactionEvent found in receipt; txId cannot be resolved unambiguously'); | ||
| } | ||
| return fromEvent; | ||
| } |
Contributor
There was a problem hiding this comment.
Missing TransactionEvent now bypasses tx-status validation.
When txId cannot be extracted, downstream assertions skip tx-record checks and continue. That can let failed batch operations pass silently when logs are unavailable.
💡 Suggested fix
protected async resolveTxIdForControllerOperation(receipt: any): Promise<bigint | null> {
const fromEvent = this.extractTxIdFromReceipt(receipt);
if (fromEvent == null) {
- console.log(' ⚠️ No TransactionEvent found in receipt; txId cannot be resolved unambiguously');
+ throw new Error('No TransactionEvent found in receipt; cannot safely validate tx status');
}
return fromEvent;
}Also applies to: 751-755
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/sanity-sdk/guard-controller/base-test.ts` around lines 965 - 971, The
resolveTxIdForControllerOperation method currently returns null when
extractTxIdFromReceipt yields no TransactionEvent, which allows downstream
assertions to skip tx-status validation; change this behavior so that when
fromEvent is null the method throws a descriptive error (or otherwise fails the
test) instead of returning null, referencing resolveTxIdForControllerOperation
and extractTxIdFromReceipt so callers cannot bypass tx-record checks — update
any identical logic at the other occurrence (around lines 751-755) as well.
Comment on lines
+738
to
+751
| const history: any[] = await (this.guardController as any).getTransactionHistory(1n, largeUpperBound); | ||
| // Find the most recent record matching requester, target, and execution selector for this mint. | ||
| for (let i = history.length - 1; i >= 0; i--) { | ||
| const tx = history[i] as any; | ||
| const params = tx.params ?? tx[3] ?? {}; | ||
| const requesterHist = params.requester ?? params.requesterReturn ?? params[1]; | ||
| const targetHist = params.target ?? params.targetReturn ?? params[2]; | ||
| const execSelHist = params.executionSelector ?? params.executionSelectorReturn ?? params[6]; | ||
| const requesterMatch = | ||
| requesterHist && String(requesterHist).toLowerCase() === mintRequestor.address.toLowerCase(); | ||
| const targetMatch = targetHist && String(targetHist).toLowerCase() === token.toLowerCase(); | ||
| const selectorMatch = | ||
| execSelHist && String(execSelHist).toLowerCase() === ERC20_MINT_SELECTOR.toLowerCase(); | ||
| if (requesterMatch && targetMatch && selectorMatch) { |
Contributor
There was a problem hiding this comment.
Fix tx-history fallback indexing and matching before inferring txId.
The tuple fallbacks are misaligned (requester/target/executionSelector indexes), and matching only requester+target+selector can select a stale tx in a busy history.
💡 Suggested fix
- const history: any[] = await (this.guardController as any).getTransactionHistory(1n, largeUpperBound);
+ const history: any[] = await (this.guardController as any).getTransactionHistory(1n, largeUpperBound);
// Find the most recent record matching requester, target, and execution selector for this mint.
for (let i = history.length - 1; i >= 0; i--) {
const tx = history[i] as any;
const params = tx.params ?? tx[3] ?? {};
- const requesterHist = params.requester ?? params.requesterReturn ?? params[1];
- const targetHist = params.target ?? params.targetReturn ?? params[2];
- const execSelHist = params.executionSelector ?? params.executionSelectorReturn ?? params[6];
+ const requesterHist = params.requester ?? params.requesterReturn ?? params[0];
+ const targetHist = params.target ?? params.targetReturn ?? params[1];
+ const opTypeHist = params.operationType ?? params.operationTypeReturn ?? params[4];
+ const execSelHist = params.executionSelector ?? params.executionSelectorReturn ?? params[5];
+ const execParamsHist = params.executionParams ?? params.executionParamsReturn ?? params[6];
const requesterMatch =
requesterHist && String(requesterHist).toLowerCase() === mintRequestor.address.toLowerCase();
const targetMatch = targetHist && String(targetHist).toLowerCase() === token.toLowerCase();
+ const opTypeMatch =
+ opTypeHist && String(opTypeHist).toLowerCase() === operationType.toLowerCase();
const selectorMatch =
execSelHist && String(execSelHist).toLowerCase() === ERC20_MINT_SELECTOR.toLowerCase();
- if (requesterMatch && targetMatch && selectorMatch) {
+ const paramsMatch =
+ execParamsHist && String(execParamsHist).toLowerCase() === executionParams.toLowerCase();
+ if (requesterMatch && targetMatch && opTypeMatch && selectorMatch && paramsMatch) {
const foundId = tx.txId ?? tx.txIdReturn ?? tx[0];
if (foundId != null) {
txId = BigInt(foundId);As per coding guidelines, **/*.{ts,tsx} should “Expose a type-safe TypeScript SDK (interfaces and classes) for contract interaction … handle async contract calls using typed methods, returning typed results”.
Also applies to: 752-755
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/sanity-sdk/guard-controller/erc20-mint-controller-tests.ts` around
lines 738 - 751, The tuple fallbacks for extracting
requester/target/executionSelector from getTransactionHistory are using the
wrong index order and the selector-only match can pick a stale tx; update the
fallbacks to match the actual tuple layout returned by getTransactionHistory
(use params.requesterReturn ?? params[0] for requester, params.targetReturn ??
params[1] for target, params.executionSelectorReturn ?? params[6] for execution
selector) and tighten the match before inferring txId by also comparing the mint
amount/nonce or another unique field from the mint request (e.g., params.amount
?? params[3]) and/or tx.timestamp/txId/status so you only accept the exact mint
record that matches ERC20_MINT_SELECTOR and the request payload in
getTransactionHistory/guardController entries.
Comment on lines
+833
to
+842
| if (/Missing or invalid parameters/i.test(combined)) { | ||
| console.log( | ||
| ' ⏭️ Mint 100 BASIC via 3-step flow skipped: RPC rejected payload with "Missing or invalid parameters"; treating as environment limitation.' | ||
| ); | ||
| this.mintFlowSkipped = true; | ||
| this.skipTest( | ||
| 'Mint 100 BASIC via 3-step flow skipped due to RPC "Missing or invalid parameters" (likely environment limitation).' | ||
| ); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Narrow the environment-limitation skip condition.
A plain message regex for "Missing or invalid parameters" can skip real regressions. Gate this skip using RPC error code/context (e.g., -32602) before downgrading to skipped.
💡 Suggested fix
- if (/Missing or invalid parameters/i.test(combined)) {
+ const rpcCode = (error as any)?.code ?? (error as any)?.cause?.code;
+ if (rpcCode === -32602 && /Missing or invalid parameters/i.test(combined)) {
console.log(
' ⏭️ Mint 100 BASIC via 3-step flow skipped: RPC rejected payload with "Missing or invalid parameters"; treating as environment limitation.'
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/sanity-sdk/guard-controller/erc20-mint-controller-tests.ts` around
lines 833 - 842, The current check treats any error message matching /Missing or
invalid parameters/i as an environment limitation; instead, only downgrade to a
skipped test when the RPC error indicates the specific invalid-params code
(e.g., rpc error code -32602) or equivalent context. In the block around the
variables combined, this.mintFlowSkipped and skipTest, inspect the original RPC
error object (not just the combined string) and require rpcError.code === -32602
(or combined includes '"code":-32602') before setting mintFlowSkipped and
calling skipTest; if the code is absent, preserve the failure path and surface
the error. Ensure logging still prints the full error and context when you do
skip so debugging info is retained.
…ntation This commit introduces a new `AGENTS.md` file that serves as a briefing document for AI agents working on the Bloxchain protocol. It outlines the tech stack, project structure, commands for agents, documentation rules, code style constraints, and boundaries for agent actions. Additionally, it updates the `getting-started.md` and `account-pattern.md` documents to reflect the new account-based contract approach, enhancing clarity and usability for developers. The `core-contract-graph.md` is also added to illustrate the relationships between core Solidity contracts and the TypeScript SDK, providing a visual representation of the architecture.
This commit refines the TypeScript SDK documentation by correcting references to the account address type and enhancing the clarity of transaction approval instructions. It also updates links in the index and README files to ensure proper navigation within the documentation. These changes improve the overall usability and accuracy of the SDK documentation, aligning it with the latest codebase updates.
docs: add comprehensive agent briefing document and update SDK docume…
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.
Summary by CodeRabbit
Documentation
Chores