feat(collateral): harden dual-state collateral contract and validator integration#372
Open
feat(collateral): harden dual-state collateral contract and validator integration#372
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughIntegrates dual-state collateral (TAO + alpha) across contracts, validator, CLI, and scripts; replaces grace-period exclusion with preference-based logic and RPC-driven background syncing; adds comprehensive CLI collateral commands, localnet deployment and E2E scripts, and large Solidity contract refactors (upgradeable pattern, trustee role, SHA‑256 evidence). Changes
Sequence Diagram(s)sequenceDiagram
participant Validator as Validator(Service)
participant RPC as Blockchain RPC
participant DB as Database
participant Evaluator as CollateralEvaluator
participant API as TokenPriceAPI
loop periodic sync
Validator->>RPC: get_finalized_block()
RPC-->>Validator: block_number
Validator->>RPC: get_all_collaterals_at_block(block)
RPC-->>Validator: list<(minerHotkey,nodeId,tao,alpha)>
Validator->>DB: sync_all_collateral_nodes(nodes)
DB-->>Validator: ack
Validator->>RPC: get_all_reclaims_at_block(block)
RPC-->>Validator: reclaims
Validator->>DB: sync_all_reclaims(reclaims)
DB-->>Validator: ack
Validator->>DB: get_all_nodes_with_gpu_info()
DB-->>Validator: nodes_with_gpu
Validator->>API: fetch_alpha_price()
API-->>Validator: alpha_price
loop per node
Validator->>Evaluator: compute_preference(node, alpha_price)
Evaluator-->>Validator: CollateralPreference
end
Validator->>DB: batch_update_collateral_preferences(updates)
DB-->>Validator: ack
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
itzlambda
force-pushed
the
collateral-integration
branch
2 times, most recently
from
bb23d5d to
5a6753e
Compare
Fix finalizeReclaim to decrement alphaCollateralUnderPendingReclaims, cap transfers to available balance instead of reverting after slash, and enforce proper CEI ordering. Fix denyReclaimRequest to recognize alpha-only reclaims. Add regression tests for partial/full slash scenarios and pending counter lifecycle.
…reclaims Check collateralUnderPendingReclaims and alphaCollateralUnderPendingReclaims before clearing nodeToMiner in both finalizeReclaim and slashCollateral, closing a theft vector where a new depositor's funds could be drained by stale reclaim finalizations. Added regression tests for the attack path.
…taking details Add coldkey/hotkey model, StakingV2 function reference, alpha token economics, dual-mode collateral state, and delegatecall vs call semantics.
Add uint256[50] storage gap for upgrade safety and guard deposit() against msg.value == 0 && alphaAmount == 0 to prevent ownership griefing without collateral.
…s are zero After a full slash with a pending reclaim, denyReclaimRequest zeroed pending trackers but never cleared nodeToMiner, permanently locking the node to the old owner.
itzlambda
force-pushed
the
collateral-integration
branch
from
56bbbbb to
3931232
Compare
The variable holds the validator's hotkey where all alpha collateral is consolidated, not a generic contract key.
Clarify that withdrawAlpha uses transferStake (changes coldkey ownership only), not removeStake. Alpha remains staked under VALIDATOR_HOTKEY. Also fix TAO slash description: sent to address(0), not trustee.
Mainnet whitelist is already disabled (open to all). Localnet now matches by auto-disabling via sudo during initialization.
- set Foundry solc to 0.8.24 and evm_version to cancun - align contract/script/test pragmas to ^0.8.24 - fix deploy/query helper script shebang lines
Use OpenZeppelin ReentrancyGuardUpgradeable on collateral lifecycle entrypoints. Also block constructor-based first-claim bypasses and verify alpha slash transfers to trustee coldkey with regression tests.
itzlambda
changed the title
Enforce required env inputs in the Foundry deploy script for trustee/admin/hotkey and numeric params, removing unsafe defaults. Update collateral docs and audit report to reflect trustee-only authority semantics and remediation status.
Refresh generated collateral ABI and embedded bytecode from the current Solidity build, and align miner tests with lowerCamel getter bindings.
Switch trustee-gated paths to TRUSTEE_ROLE, replace custom math with OpenZeppelin Math/SafeCast, and replace string requires with custom errors while keeping current operational flow.
Automate localnet contract bring-up by creating/funding a deployer wallet, deploying proxy+implementation, and generating scripts/collateral/.env.local. Also replace forge script deployment with deploy.sh/forge create and update collateral/localnet docs and configs.
…isplay DRY up time-remaining formatting into format_time_remaining() and add Display impl for SendToken to simplify interactive token selection.
Replace Option<PathBuf> with PathBuf for key_file in collateral commands, letting clap enforce the requirement instead of manual validation.
Replace parse_hotkey (hex-based) with existing parse_ss58_address so users can pass natural SS58 hotkeys (e.g. 5GQfz…) directly.
…r convention Bittensor's official precompile interfaces use `hotkey` as the canonical term. Disambiguate via qualified naming (`minerHotkey`), consistent with `validatorHotkey` and `alphaHotkey`.
…nment Display alpha hotkey in human-readable SS58 format with "Staked under" label instead of raw hex, and align deposit summary columns consistently.
itzlambda
force-pushed
the
collateral-integration
branch
from
fff44d2 to
3f6f9a2
Compare
…ad of rental hot path Move collateral preference evaluation from per-bid-candidate lookups in RentalManager to a periodic background computation in the collateral scanner. Preferences are stored in a new `collateral_preference` column on `miner_nodes` and read directly during bid selection, eliminating the CollateralManager dependency from the rental path.
![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: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/basilica-validator/src/collateral/slash_executor.rs (1)
267-287:⚠️ Potential issue | 🟠 MajorDon't quantize
slash_fractionto whole percent.
* 100+round()changes the configured policy before you apply it:0.006becomes 1%,0.0149becomes 1%, and0.995becomes 100%. That can materially over- or under-slash on-chain. Keep finer precision here (for example basis points) and only clamp to a 1-unit minimum at the end.⚙️ Proposed fix
- let numerator = (self.config.slash_fraction * Decimal::from(100u64)) + let numerator = (self.config.slash_fraction * Decimal::from(10_000u64)) .round() .to_u64() .unwrap_or(0); if numerator == 0 { return U256::from(1u64); } - if numerator >= 100 { + if numerator >= 10_000 { return collateral; } - let amount = collateral * U256::from(numerator) / U256::from(100u64); + let amount = collateral * U256::from(numerator) / U256::from(10_000u64);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/basilica-validator/src/collateral/slash_executor.rs` around lines 267 - 287, compute_slash_amount currently quantizes self.config.slash_fraction by multiplying by 100 and rounding, which changes policy; instead, keep full precision by converting collateral into a Decimal, multiply it directly by self.config.slash_fraction, then convert the resulting Decimal back to a U256 (floor/round toward zero) and clamp: if self.config.slash_fraction.is_zero() return U256::from(1u64) (after the zero check), if self.config.slash_fraction >= Decimal::ONE return collateral, and after computing amount ensure amount >= 1 (return U256::from(1u64) if zero) and never exceed collateral; update compute_slash_amount to use Decimal arithmetic on collateral and avoid the *100 + round() pathway so tiny fractions are preserved.crates/basilica-validator/tests/collateral_e2e.rs (1)
164-169:⚠️ Potential issue | 🟡 MinorThe checksum assertion no longer proves SHA-256 correctness.
assert_ne!(calls[0].checksum, [0u8; 32])only shows that some non-zero bytes were passed through. A wrong payload hash or padded legacy digest would still pass. Please compare it to the expected 32-byte SHA-256 digest for the evidence payload this test writes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/basilica-validator/tests/collateral_e2e.rs` around lines 164 - 169, Replace the weak inequality check on calls[0].checksum with an exact SHA-256 comparison: compute the expected 32-byte digest of the evidence payload that this test writes (use the same payload bytes used earlier in the test), e.g. by hashing that payload with sha2::Sha256 and taking the 32-byte result, then assert_eq!(calls[0].checksum, expected_digest); keep the other assertions (alpha_amount, private_key, url) intact and use calls() / calls[0].checksum to locate the assertion to update.
🟡 Minor comments (12)
scripts/lib/common.sh-66-75 (1)
66-75:⚠️ Potential issue | 🟡 MinorValidate the hotkey format before returning it.
This helper advertises a
0x-prefixedbytes32, but Line 74 just echoes whateverpublicKeycontains. Invalid wallet JSON will slip through here and fail much later in the contract/CLI path.Proposed hardening
read_hotkey() { local pubfile="$1" if [[ ! -f "$pubfile" ]]; then log_error "Hotkey file not found: $pubfile" return 1 fi - python3 -c "import json,sys; print(json.load(sys.stdin)['publicKey'])" < "$pubfile" + python3 - "$pubfile" <<'PY' +import json, re, sys + +path = sys.argv[1] +try: + with open(path, "r", encoding="utf-8") as fh: + value = json.load(fh)["publicKey"] +except (OSError, ValueError, KeyError) as exc: + sys.stderr.write(f"Failed to read publicKey from {path}: {exc}\n") + raise SystemExit(1) + +if not isinstance(value, str) or not re.fullmatch(r"0x[0-9a-fA-F]{64}", value): + sys.stderr.write(f"Invalid publicKey in {path}: expected 0x-prefixed 32-byte hex\n") + raise SystemExit(1) + +print(value) +PY }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lib/common.sh` around lines 66 - 75, In read_hotkey(), validate and normalize the wallet JSON publicKey before printing: load publicKey, ensure it is hex characters only, prepend "0x" if missing, and verify it is a bytes32 hex string (0x followed by exactly 64 hex chars); if validation fails, emit a clear log_error and return non-zero. Update the inline python3 snippet (or replace with a short bash validation) to perform the checks and normalization and only print the validated 0x-prefixed bytes32 value, referencing the read_hotkey function and its pubfile argument so the failing path is detected immediately.scripts/collateral/bigint_compare.py-32-41 (1)
32-41:⚠️ Potential issue | 🟡 MinorHandle signed values explicitly in the formatters.
//and%floor on negative integers in Python, sofmt_tao(-1)currently renders as-1.999999999999999999 TAOinstead of-0.000000000000000001. The same bug exists infmt_alpha, which makes signed deltas misleading.💡 Proposed fix
+def _format_units(value: int, scale: int, decimals: int, symbol: str, raw_unit: str) -> str: + sign = "-" if value < 0 else "" + raw = abs(value) + whole = raw // scale + frac = raw % scale + return f"{sign}{whole}.{frac:0{decimals}d} {symbol} ({value} {raw_unit})" + + def fmt_tao(wei: int) -> str: - whole = wei // 10**18 - frac = wei % 10**18 - return f"{whole}.{frac:018d} TAO ({wei} wei)" + return _format_units(wei, 10**18, 18, "TAO", "wei") def fmt_alpha(rao: int) -> str: - whole = rao // 10**9 - frac = rao % 10**9 - return f"{whole}.{frac:09d} alpha ({rao} rao)" + return _format_units(rao, 10**9, 9, "alpha", "rao")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/collateral/bigint_compare.py` around lines 32 - 41, The formatters fmt_tao and fmt_alpha mishandle negative inputs because // and % floor toward negative infinity; update both functions to detect the sign (e.g., sign = "-" if value < 0 else ""), operate on the absolute value (abs(wei) / abs(rao)) to compute whole and frac, then prefix the sign to the formatted string so fmt_tao(-1) becomes -0.000...001 TAO and similarly for fmt_alpha; keep types and return format unchanged and apply this change to both fmt_tao and fmt_alpha.scripts/collateral/bigint_compare.py-44-72 (1)
44-72:⚠️ Potential issue | 🟡 MinorEnforce exact arity and convert parse failures into usage errors.
Right now
fmt_tao 1 extrasucceeds, andfmt_tao not-a-numberraises a traceback instead of returning the documented usage exit code2. For a script that will be called from shell tooling, that makes invocation bugs harder to detect.💡 Proposed fix
def main() -> int: - if len(sys.argv) < 3: + if len(sys.argv) < 2: print(__doc__, file=sys.stderr) return 2 cmd = sys.argv[1] - - if cmd == "fmt_tao": - print(fmt_tao(parse_int(sys.argv[2]))) - return 0 - - if cmd == "fmt_alpha": - print(fmt_alpha(parse_int(sys.argv[2]))) - return 0 - - if len(sys.argv) < 4: - print(__doc__, file=sys.stderr) - return 2 - - a = parse_int(sys.argv[2]) - b = parse_int(sys.argv[3]) + try: + if cmd == "fmt_tao": + if len(sys.argv) != 3: + print(__doc__, file=sys.stderr) + return 2 + print(fmt_tao(parse_int(sys.argv[2]))) + return 0 + + if cmd == "fmt_alpha": + if len(sys.argv) != 3: + print(__doc__, file=sys.stderr) + return 2 + print(fmt_alpha(parse_int(sys.argv[2]))) + return 0 + + if cmd not in {"gt", "gte", "eq", "lt", "lte"} or len(sys.argv) != 4: + print(__doc__, file=sys.stderr) + return 2 + + a = parse_int(sys.argv[2]) + b = parse_int(sys.argv[3]) + except ValueError as exc: + print(f"Invalid integer input: {exc}", file=sys.stderr) + return 2 ops = {"gt": a > b, "gte": a >= b, "eq": a == b, "lt": a < b, "lte": a <= b} - - if cmd not in ops: - print(f"Unknown command: {cmd}", file=sys.stderr) - return 2 return 0 if ops[cmd] else 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/collateral/bigint_compare.py` around lines 44 - 72, The main() function accepts wrong arity and lets parse_int raise exceptions; update it to enforce exact argument counts for each command and to convert parse_int failures into usage errors (exit code 2) by catching ValueError (or the specific exception parse_int raises), printing __doc__ (or a clear usage message) to stderr and returning 2; specifically check that fmt_tao and fmt_alpha are called with exactly one operand and the comparison commands ("gt","gte","eq","lt","lte") are called with exactly two operands before calling parse_int(fmt/parse), and wrap calls to parse_int used for fmt_tao, fmt_alpha and the two comparison operands in try/except to handle invalid numbers by returning 2.crates/collateral-contract/README.md-187-187 (1)
187-187:⚠️ Potential issue | 🟡 MinorUse actual SHA-256 examples for
--url-content-sha256.These samples are duplicated MD5-looking digests (
d41d8cd…,5d41402…,098f6bcd…) stretched to 64 hex chars. That contradicts the SHA-256 migration this README is documenting and makes the evidence format easy to misunderstand.Also applies to: 212-212, 234-234
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/collateral-contract/README.md` at line 187, Replace the bogus MD5-like 64-hex placeholders used for the --url-content-sha256 examples in the README (instances near the shown diff and at the other noted locations) with real SHA-256 example digests (64 hex characters produced by a real SHA-256 of sample content), so examples match the documented migration to SHA-256 and avoid confusion; update each occurrence of the flag's sample value to a valid SHA-256 hex string (e.g., digest of the actual example content) while leaving the surrounding command text unchanged.crates/collateral-contract/README.md-160-161 (1)
160-161:⚠️ Potential issue | 🟡 MinorAvoid private-key-shaped literals in docs.
Line 161 uses a realistic 32-byte hex key, which already trips secret scanning and normalizes exporting raw keys into shell history. An obvious placeholder like
0x<YOUR_PRIVATE_KEY>is safer and avoids tooling noise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/collateral-contract/README.md` around lines 160 - 161, Replace the realistic 32-byte hex literal used for the PRIVATE_KEY example with a non-secret placeholder to avoid secret-scanning and accidental key leaks; specifically update the example that exports PRIVATE_KEY (the line showing export PRIVATE_KEY=0x1234...ef12) to use a clearly non-sensitive placeholder such as 0x<YOUR_PRIVATE_KEY> or <REPLACE_WITH_PRIVATE_KEY> and add a short comment advising users not to paste real keys into shell history.crates/collateral-contract/README.md-40-53 (1)
40-53:⚠️ Potential issue | 🟡 MinorHonor
RPC_URLin the manual deployment example.This snippet hard-codes the testnet RPC twice, even though the repo now documents
RPC_URLas an input for the deployment flow. Copy/pasting it will silently ignore localnet/custom endpoints and undermines the new override behavior.Suggested doc fix
+export RPC_URL=https://test.chain.opentensor.ai + impl_out="$(forge create src/CollateralUpgradeable.sol:CollateralUpgradeable \ - --rpc-url https://test.chain.opentensor.ai \ + --rpc-url "$RPC_URL" \ --private-key "$PRIVATE_KEY" \ --broadcast)" ... proxy_out="$(forge create lib/openzeppelin-contracts/contracts/proxy/ERC1967/ERC1967Proxy.sol:ERC1967Proxy \ - --rpc-url https://test.chain.opentensor.ai \ + --rpc-url "$RPC_URL" \ --private-key "$PRIVATE_KEY" \ --broadcast \ --constructor-args "$IMPLEMENTATION_ADDRESS" "$INIT_DATA")"Also applies to: 370-374
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/collateral-contract/README.md` around lines 40 - 53, The README hard-codes the RPC endpoint in the forge/cast commands (see invocations that set impl_out and proxy_out) which ignores the documented RPC_URL override; update those commands to use the RPC_URL environment variable instead of the literal https://test.chain.opentensor.ai (also update the second occurrence noted around the proxy creation block) so both the impl_out (forge create ... --rpc-url ...) and proxy_out forge create lines read --rpc-url "$RPC_URL" to honor local/custom endpoints.scripts/collateral/README.md-51-72 (1)
51-72:⚠️ Potential issue | 🟡 MinorAdd documentation for the
min-alpha-collateral-increasequery command.The README documents
min-collateral-increaseandcollaterals, which are correct and will work. However, themin-alpha-collateral-increasequery command exists and is not documented here. Include it alongside the TAO variant to give users visibility into both minimum collateral thresholds:cc query min-collateral-increase cc query min-alpha-collateral-increase🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/collateral/README.md` around lines 51 - 72, The README is missing documentation for the min-alpha-collateral-increase query; add it next to the existing min-collateral-increase entry so both TAO and alpha variants are shown. Update the “Contract metadata” snippet to list both commands (cc query min-collateral-increase and cc query min-alpha-collateral-increase) and, if applicable, mirror any example usage or explanation you have for min-collateral-increase so users can see both minimum collateral thresholds; ensure the new line appears alongside the existing cc query min-collateral-increase and cc query collaterals entries.scripts/localnet/start.sh-236-236 (1)
236-236:⚠️ Potential issue | 🟡 Minorsed -i syntax is macOS-specific and will fail on Linux.
The
sed -i ''syntax works on macOS but Linux's GNU sed requiressed -iwithout the empty string argument. This will cause the script to fail on Linux systems.🔧 Proposed fix for cross-platform sed
- sed -i '' "s|^contract_address = .*|contract_address = \"${address}\"|" "$config_file" + # Cross-platform sed in-place edit + if [[ "$OSTYPE" == "darwin"* ]]; then + sed -i '' "s|^contract_address = .*|contract_address = \"${address}\"|" "$config_file" + else + sed -i "s|^contract_address = .*|contract_address = \"${address}\"|" "$config_file" + fiAlternatively, use a temp file approach that works on both platforms:
- sed -i '' "s|^contract_address = .*|contract_address = \"${address}\"|" "$config_file" + sed "s|^contract_address = .*|contract_address = \"${address}\"|" "$config_file" > "${config_file}.tmp" && mv "${config_file}.tmp" "$config_file"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/localnet/start.sh` at line 236, The sed invocation using `sed -i '' "s|^contract_address = .*|contract_address = \"${address}\"|" "$config_file"` is macOS-specific and will fail on GNU sed; replace it with a cross-platform approach: create a temporary file (via mktemp or similar), run sed without in-place editing to write the modified content into that temp file, then atomically move/replace the original `$config_file` with the temp file; reference the existing variables `config_file` and `address` and the current sed substitution to ensure the same replacement semantics.crates/basilica-cli/src/cli/args.rs-339-342 (1)
339-342:⚠️ Potential issue | 🟡 MinorThread the global
--jsonmode into the collateral path.This is the only top-level command branch that drops
self.json, sobasilica --json collateral ...cannot honor the CLI's global output contract.💡 Proposed change
- handlers::collateral::handle_collateral(action, key_file).await?; + handlers::collateral::handle_collateral(action, key_file, self.json).await?;That will require updating
handle_collateraland any downstream helpers that format output.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/basilica-cli/src/cli/args.rs` around lines 339 - 342, The collateral command branch is not passing the global JSON flag (self.json) into the collateral handler, so calls to Commands::Collateral { action, key_file } should call handlers::collateral::handle_collateral with the json mode; update the call site to pass self.json (or equivalent) and then update handlers::collateral::handle_collateral signature and all downstream helper functions that format output to accept a json: bool (or OutputMode) parameter and switch formatting based on that flag so `basilica --json collateral ...` honors the global output contract.crates/collateral-contract/foundry.toml-5-6 (1)
5-6:⚠️ Potential issue | 🟡 MinorAdd
FOUNDRY_PROFILE=localto README.md forge test examples.The README.md documentation at lines 23, 359, 362, 365, and 368 shows bare
forge testcommands that will compile against the default Cancun EVM. Update these to includeFOUNDRY_PROFILE=localso developers following the docs test against the London profile that matches the localnet configuration.The actual deployment scripts already use
FOUNDRY_PROFILE=localcorrectly indeploy.sh, but documentation examples should be consistent.Examples to update in README.md
# Current (incorrect for localnet) forge test forge test --match-test testDeposit forge test -vvv forge test --gas-report # Should be FOUNDRY_PROFILE=local forge test FOUNDRY_PROFILE=local forge test --match-test testDeposit FOUNDRY_PROFILE=local forge test -vvv FOUNDRY_PROFILE=local forge test --gas-reportAlso applies to: 17-20
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/collateral-contract/foundry.toml` around lines 5 - 6, Update the README.md test command examples by prefixing every bare "forge test" invocation (including variants with "--match-test testDeposit", "-vvv", and "--gas-report") with "FOUNDRY_PROFILE=local" so documentation matches the localnet London profile used by deploy.sh; replace lines that show "forge test", "forge test --match-test testDeposit", "forge test -vvv", and "forge test --gas-report" with "FOUNDRY_PROFILE=local forge test" equivalents throughout the file.crates/basilica-cli/src/cli/handlers/collateral.rs-392-413 (1)
392-413:⚠️ Potential issue | 🟡 MinorApply
--hotkeybefore rendering aggregate status.The aggregate path ignores
params.hotkey, sobasilica collateral status --hotkey ...still prints every collateral row and every pending reclaim. Filter both collections before building the tables.Also applies to: 423-458
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/basilica-cli/src/cli/handlers/collateral.rs` around lines 392 - 413, The listing ignores params.hotkey; before mapping `all` to `rows` filter `all` by `params.hotkey` (if Some) using the same key format as `hotkey_to_ss58(&n.miner_hotkey)` so only matching collaterals are displayed; likewise locate the pending-reclaims collection (used later around the 423-458 region) and filter that collection by `params.hotkey` before building its table so pending reclaims are also restricted to the requested hotkey. Ensure you apply the filter prior to constructing `CollateralRow` and the pending-reclaim rows so rendering respects the CLI `--hotkey` flag.crates/basilica-validator/src/collateral/collateral_scan.rs-116-121 (1)
116-121:⚠️ Potential issue | 🟡 MinorPartial sync failures leave inconsistent state.
If
sync_all_collateral_nodessucceeds butsync_all_reclaimsfails (or vice versa), the database will be in an inconsistent state with mismatched node and reclaim data. Additionally,update_last_scanned_block_numberis called after both operations, so a failure in reclaims will prevent the block number update but leave nodes already synced.Consider wrapping all three operations in a transaction or using a more atomic approach where all operations succeed or fail together.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/basilica-validator/src/collateral/collateral_scan.rs` around lines 116 - 121, The three persistence calls (self.persistence.sync_all_collateral_nodes, self.persistence.sync_all_reclaims, and self.persistence.update_last_scanned_block_number) must be executed atomically to avoid partial commits; wrap them in a single database transaction via the persistence layer (e.g., begin_transaction / commit / rollback or transaction(async |tx| { ... }) API), call sync_all_collateral_nodes and sync_all_reclaims against the transaction/tx and then update_last_scanned_block_number on success, commit the transaction, and ensure any error from any of these methods triggers a rollback and returns the error instead of leaving partially applied state.
🧹 Nitpick comments (19)
scripts/subtensor-local/init.py (1)
91-91: Consider addingwait_for_finalization=Truefor consistency.Other extrinsic submissions in this file (e.g.,
register_subnet,burned_register,start_call) use bothwait_for_inclusion=Trueandwait_for_finalization=True. For consistency and to ensure the whitelist is fully disabled before any dependent operations:Suggested change
- receipt = substrate.submit_extrinsic(extrinsic, wait_for_inclusion=True) + receipt = substrate.submit_extrinsic(extrinsic, wait_for_inclusion=True, wait_for_finalization=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/subtensor-local/init.py` at line 91, The call to substrate.submit_extrinsic that assigns receipt currently only uses wait_for_inclusion=True; update that call (the substrate.submit_extrinsic(...) that returns receipt) to also pass wait_for_finalization=True so it matches other submissions like register_subnet, burned_register, and start_call and ensures the whitelist-disable extrinsic is finalized before proceeding.scripts/localnet/init-subnet.sh (2)
265-265: Balance parsing may mishandle scientific notation.The
sed 's/[^0-9.]//g'approach strips all non-numeric characters, which could misparse values in scientific notation (e.g.,"1e10"becomes"110"instead of10000000000). For a localnet script where balances are typically whole numbers, this is unlikely to cause issues, but worth noting.📝 Optional: Use jq's numeric coercion instead
- current_balance=$(echo "$balance_json" | jq -r '.balance.free // "0"' 2>/dev/null | sed 's/[^0-9.]//g') + current_balance=$(echo "$balance_json" | jq -r '(.balance.free // 0) | tonumber' 2>/dev/null || echo "0")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/localnet/init-subnet.sh` at line 265, The current_balance assignment strips non-digits with sed which mangles scientific notation; update the parsing to let jq coerce to a numeric value instead of post-processing with sed: change the pipeline that reads balance_json and sets current_balance (the line referencing balance_json and current_balance) to use jq's numeric coercion (e.g., use '.balance.free // 0 | tonumber' or similar) and remove the sed step so the value is emitted as a proper numeric string (fallback to 0 on missing values).
357-358: Hardcoded sleep is coupled to tempo configuration.The
sleep 45assumes tempo=20 with ~2s block times. If the tempo setting on line 161 fails (it only prints a WARNING) or block times vary, the sleep may be insufficient for subtoken activation. For a localnet script this is acceptable, but a polling approach would be more robust.📝 Optional: Poll for subtoken activation instead of fixed sleep
echo " Waiting for next epoch to activate subtokens (tempo=20, ~40s)..." -sleep 45 +# Poll until subtokens are active (up to 60s) +for i in {1..30}; do + if uvx --python 3.12 --from bittensor-cli btcli subnet list --network local --json-output 2>/dev/null | jq -e '.[] | select(.netuid == 1 and .subtokens_enabled == true)' >/dev/null 2>&1; then + echo " Subtokens activated" + break + fi + sleep 2 +done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/localnet/init-subnet.sh` around lines 357 - 358, The script uses a hardcoded sleep ("sleep 45") tied to the tempo setting; replace this with a polling loop that repeatedly queries the subnet/subtoken activation state until it becomes active or a sensible timeout is reached (e.g., 2-3× expected epoch time), rather than sleeping a fixed 45s. Locate the "sleep 45" call and the tempo configuration (the printed WARNING about tempo around line 161) and implement: poll every few seconds (using sleep N) calling the existing CLI/API used elsewhere in the script to check subtoken activation status, break when active, and fail with a clear error if timeout elapses; optionally compute the timeout from the tempo variable if present. Ensure the loop is idempotent and logs progress instead of relying on the fixed "sleep 45".crates/collateral-contract/test/IStakingIntegration.t.sol (3)
213-218: Validate the SHA-256 evidence path in the slash test.The PR explicitly changes slash evidence handling to SHA-256, but this test only checks balances. If checksum persistence/emission regressed, this would still pass. Consider asserting the emitted slash event or stored slash record includes
TEST_SHA256.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/collateral-contract/test/IStakingIntegration.t.sol` around lines 213 - 218, Update the test to validate that the SHA-256 evidence is persisted/emitted by asserting the slash evidence contains TEST_SHA256 after calling collateral.slashCollateral(HOTKEY_1, EXECUTOR_ID_1, 0, slashAlphaAmount, TEST_URL, TEST_SHA256); for example, capture the emitted Slash event (or read the stored slash record) and add an assertion that the event/record's checksum or evidence field equals TEST_SHA256 so the test fails if SHA-256 handling regresses; locate the call to collateral.slashCollateral and add the event assertion (or storage lookup) immediately after the balance assertions.
194-199: Assert thatfinalizeReclaimclears internal alpha accounting.This test proves stake moves back to Alice, but it never checks that
collateral.alphaCollaterals(HOTKEY_1, EXECUTOR_ID_1)is zero afterward. A stale internal balance would still let this pass while breaking later slash/eligibility logic.Suggested assertion
collateral.finalizeReclaim(0); + assertEq(collateral.alphaCollaterals(HOTKEY_1, EXECUTOR_ID_1), 0); assertEq(mockStaking.getStake(VALIDATOR_HOTKEY, contractColdkey, NETUID), 0); assertEq(mockStaking.getStake(VALIDATOR_HOTKEY, ALICE_COLDKEY, NETUID), ALPHA_AMOUNT);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/collateral-contract/test/IStakingIntegration.t.sol` around lines 194 - 199, After calling collateral.finalizeReclaim(0) and the existing stake assertions, add an assertion that the internal alpha accounting is cleared by checking collateral.alphaCollaterals(...) equals zero; specifically call alphaCollaterals with the validator hotkey constant (e.g., VALIDATOR_HOTKEY or HOTKEY_1) and the executor id constant (EXECUTOR_ID_1) to ensure collateral.alphaCollaterals(VALIDATOR_HOTKEY, EXECUTOR_ID_1) == 0 is asserted immediately after finalizeReclaim.
40-50: Tighten the mock precompile's mutator surface.
applyTransferStake,applyMoveStake, andsetStakeare callable on the etched precompile address, so any caller can bypass the caller-derived coldkey path and mutate stake state directly. That makes this mock less strict than the real integration and can hide selector/caller-context bugs in the contract under test.Also applies to: 70-80, 87-89
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/collateral-contract/test/IStakingIntegration.t.sol` around lines 40 - 50, The three mutator functions (applyTransferStake, applyMoveStake, setStake) on the mock precompile are currently callable by any caller at the etched precompile address; tighten them by enforcing the same caller-derived coldkey check used by the real integration: add a require that msg.sender equals the canonical precompile-caller address (the expected caller constant used for this integration, e.g. ISTAKING_PRECOMPILE_CALLER) at the top of applyTransferStake, applyMoveStake and setStake before delegating to _applyTransferStake/_applyMoveStake/_setStake, so only the legitimate precompile caller can mutate stake state.crates/collateral-contract/deploy.sh (1)
29-33: Make address extraction more robust than parsing human output.Both addresses are scraped from
forge createstdout. If Foundry output changes after a successful broadcast, this script will fail late and reruns can create extra implementations/proxies. Prefer structured output if your Foundry version supports it, or at least validate against an address regex instead of only checking for non-empty strings.Also applies to: 59-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/collateral-contract/deploy.sh` around lines 29 - 33, The current extraction of IMPLEMENTATION_ADDRESS from impl_output is fragile; change the script to request structured output from forge (e.g., use --json/--silent if available) and parse the implementation and proxy addresses with a JSON parser (jq) or, if JSON isn't available, validate extracted values against an Ethereum address regex (^0x[0-9a-fA-F]{40}$) instead of only checking for non-empty strings; apply the same robust extraction/validation for the proxy address extraction (the proxy_output/PROXY_ADDRESS logic around lines 59-63) so you fail fast on invalid parses and avoid accidental redeploys.scripts/subtensor-local/docker-compose.yml (1)
3-3: Consider pinning to a specific image version instead oflatestfor reproducibility.Using the
latesttag makes builds non-reproducible—the image can change upstream without notice. Even for localnet development tooling, pinning to a specific version ensures consistent environments across the team. Theghcr.io/opentensor/subtensor-localnetimage has multiple available versions includingv3.2.8-320,v3.2.6, and other stable tags that can be used instead.♻️ Suggested fix: pin to a specific version
services: alice: - image: ghcr.io/opentensor/subtensor-localnet:latest + image: ghcr.io/opentensor/subtensor-localnet:v3.2.8-320 container_name: subtensor-alicebob: - image: ghcr.io/opentensor/subtensor-localnet:latest + image: ghcr.io/opentensor/subtensor-localnet:v3.2.8-320 container_name: subtensor-bobAlso applies to: 29-29
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/subtensor-local/docker-compose.yml` at line 3, Replace the floating image tag "ghcr.io/opentensor/subtensor-localnet:latest" with a pinned, specific version tag (e.g., "ghcr.io/opentensor/subtensor-localnet:v3.2.8-320") in the docker-compose image declaration so builds are reproducible; update the image value where it appears (the image: ghcr.io/opentensor/subtensor-localnet:latest line) and choose a stable released tag your team agrees on.crates/basilica-validator/src/grpc/registration.rs (1)
41-41: Grace period field deprecated - consider documenting proto deprecation.The
grace_period_remainingfield now always returns an empty string. If this field is no longer used, consider adding a comment noting the deprecation or updating the proto definition to mark the field as deprecated for client awareness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/basilica-validator/src/grpc/registration.rs` at line 41, Update the code handling the registration response to indicate that grace_period_remaining is deprecated: add a clear comment next to the grace_period_remaining initialization in registration.rs (the field named grace_period_remaining) stating that the field is deprecated and intentionally left as an empty string, and/or update the associated protobuf definition to mark the field as deprecated (using the deprecated option in the proto) so clients are aware; ensure the comment references the proto field name and the code path that currently sets String::new() so future maintainers know why it remains present.crates/collateral-contract/src/config.rs (1)
37-48: Consider case-insensitive parsing for better UX.The
FromStrimplementation only accepts lowercase strings ("mainnet", "testnet", "local"). Users might provide "Mainnet" or "MAINNET" from CLI or config files.♻️ Suggested improvement
impl std::str::FromStr for Network { type Err = anyhow::Error; fn from_str(s: &str) -> Result<Self, Self::Err> { - match s { + match s.to_lowercase().as_str() { "mainnet" => Ok(Network::Mainnet), "testnet" => Ok(Network::Testnet), "local" => Ok(Network::Local), other => Err(anyhow::anyhow!("Unsupported collateral network: {}", other)), } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/collateral-contract/src/config.rs` around lines 37 - 48, Update the impl of std::str::FromStr for Network (the from_str method) to perform case-insensitive parsing so inputs like "Mainnet" or "MAINNET" are accepted; e.g., normalize the input (or use eq_ignore_ascii_case) before matching the variants Network::Mainnet, Network::Testnet, and Network::Local and keep the error branch returning anyhow::anyhow! with the original input preserved in the message for clarity.scripts/collateral/e2e-validator-test.sh (1)
48-72: Remove unused constants.Several constants are defined but never used in the script:
ADDRESS_MAPPING,ZERO_ADDR,TAO_5,TAO_10. These add noise and may confuse future maintainers.♻️ Remove unused constants
-STAKING_PRECOMPILE="0x0000000000000000000000000000000000000805" -ADDRESS_MAPPING="0x000000000000000000000000000000000000080C" +STAKING_PRECOMPILE="0x0000000000000000000000000000000000000805" # Query NETUID and VALIDATOR_HOTKEY from the deployed contract (set during initialize()). NETUID="$(cast call --rpc-url "$RPC_URL" "$PROXY" "netuid()(uint16)")" VALIDATOR_HOTKEY="$(cast call --rpc-url "$RPC_URL" "$PROXY" "validatorHotkey()(bytes32)")" LOCALNET_WALLETS_DIR="${REPO_ROOT}/scripts/localnet/wallets" MINER_PUBKEY="$(read_hotkey "${LOCALNET_WALLETS_DIR}/miner_1/hotkeys/defaultpub.txt")" ZERO_BYTES32="0x0000000000000000000000000000000000000000000000000000000000000000" -ZERO_ADDR="0x0000000000000000000000000000000000000000" NODE_ID_1="$(compute_node_id "127.0.0.1")" # bytes16 NODE_ID_2="$(compute_node_id "127.0.0.2")" # bytes16 # Hex with 0x prefix — how the validator stores them in SQLite (RPC snapshot: format!("0x{}", hex::encode(...))) HEX_MINER_PUBKEY="${MINER_PUBKEY}" HEX_NODE_ID_1="${NODE_ID_1}" HEX_NODE_ID_2="${NODE_ID_2}" # TAO amounts in wei (1 TAO = 1e18 wei) TAO_1="1000000000000000000" TAO_2="2000000000000000000" -TAO_5="5000000000000000000" -TAO_10="10000000000000000000"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/collateral/e2e-validator-test.sh` around lines 48 - 72, Remove the unused constant declarations to reduce noise: delete the variables ADDRESS_MAPPING, ZERO_ADDR, TAO_5, and TAO_10 from the script (the lines that assign values to those names). Before removing, search for any references to ADDRESS_MAPPING, ZERO_ADDR, TAO_5, or TAO_10 elsewhere in the file or called helper scripts (e.g., around read_hotkey, compute_node_id usage) to ensure no dependent code breaks; if found, refactor those call sites instead of deleting. After removal, tidy nearby comments/whitespace so the remaining constants (NETUID, VALIDATOR_HOTKEY, MINER_PUBKEY, NODE_ID_1/2, HEX_*, TAO_1/2) remain clear and correctly grouped.crates/collateral-contract/test/CollateralBasic.t.sol (1)
106-114: Avoid baking raw storage slot numbers into the test harness.These helpers now depend on the exact proxy storage layout (
5,8,10, field offsets, etc.). The next state-variable insertion or reorder can make the regressions write the wrong field and fail opaquely—or pass for the wrong reason. A small harness exposing test-only setters/getters would be much more stable across upgrades.Also applies to: 459-476
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/collateral-contract/test/CollateralBasic.t.sol` around lines 106 - 114, The test currently hardcodes raw storage slot math in _nodeToMinerSlot and uses vm.store in _seedNodeOwner (and other positions) which brittlely depends on proxy layout; add a test-only setter/getter pair on the Collateral contract (e.g., functions setNodeOwner(bytes32 hotkey, bytes16 nodeId, address owner) and getNodeOwner(...)) that encapsulate the storage location, update the test to call collateral.setNodeOwner(...) instead of vm.store and remove the hardcoded slot arithmetic in _nodeToMinerSlot/_seedNodeOwner; this keeps tests stable across state-variable reorders while still allowing seeding and inspection of node owner state.scripts/collateral/integration-test.sh (1)
433-456: Assert the exact alpha balance here, not just> 0.Both deposits request a fixed
RAO_4_ALPHA, soassert_gt ... 0is too weak. A RAO/alpha unit bug or partial transfer would still pass.Proposed tightening
alpha_col="$(cast_query "$PROXY" "alphaCollaterals(bytes32,bytes16)(uint256)" "$MINER_PUBKEY" "$NODE_ID_3")" log_info " alphaCollaterals = ${alpha_col} RAO (requested ${RAO_4_ALPHA})" -assert_gt "$alpha_col" "0" "alphaCollaterals(MINER_PUBKEY, NODE_ID_3) > 0" +assert_eq "$alpha_col" "$RAO_4_ALPHA" "alphaCollaterals(MINER_PUBKEY, NODE_ID_3) == ${RAO_4_ALPHA} RAO" @@ alpha_col="$(cast_query "$PROXY" "alphaCollaterals(bytes32,bytes16)(uint256)" "$MINER_PUBKEY" "$NODE_ID_4")" log_info " alphaCollaterals = ${alpha_col} RAO" -assert_gt "$alpha_col" "0" "alphaCollaterals(MINER_PUBKEY, NODE_ID_4) > 0" +assert_eq "$alpha_col" "$RAO_4_ALPHA" "alphaCollaterals(MINER_PUBKEY, NODE_ID_4) == ${RAO_4_ALPHA} RAO"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/collateral/integration-test.sh` around lines 433 - 456, Replace the loose checks that alpha_col > 0 with exact equality to the requested amount: when you call deposit(...) with RAO_4_ALPHA and then fetch alpha_col via cast_query (the alphaCollaterals(bytes32,bytes16) calls that populate the alpha_col variable for NODE_ID_3 and NODE_ID_4), change the assert_gt "$alpha_col" "0" assertions to assert_eq "$alpha_col" "$RAO_4_ALPHA" so the test verifies the full RAO_4_ALPHA was transferred rather than just being non‑zero.crates/collateral-contract/test/NodeOwnershipEdgeCases.t.sol (1)
94-103: Avoid hard-coding proxy storage slots in this helper.This assumes
nodeToMinerwill stay at slot 5 forever. Any storage-layout change above it will make the test mutate unrelated proxy state and fail for the wrong reason. Preferstdstoreor a dedicated test-only hook on a mock implementation here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/collateral-contract/test/NodeOwnershipEdgeCases.t.sol` around lines 94 - 103, The helper functions _nodeToMinerSlot and _seedNodeOwner are hardcoding the nodeToMiner storage slot (5); replace this fragile approach by locating the mapping slot dynamically (e.g., use stdstore utilities to target the nodeToMiner mapping key or add a test-only setter on the collateral mock) and then use vm.store/stdstore to write the owner value; update _seedNodeOwner to use that dynamic slot resolution (or the mock hook) rather than the constant uint256 nodeToMinerSlot = 5 so tests don’t break if storage layout changes.crates/basilica-validator/src/config/collateral.rs (1)
93-96: Consider documenting the slash_fraction precision constraint.The validation now requires
slash_fractionto be within[0.01, 1.0], which addresses the precision concern raised in PR comments about small fractional percentages rounding to zero. Consider adding a doc comment explaining this constraint and why sub-1% fractions are disallowed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/basilica-validator/src/config/collateral.rs` around lines 93 - 96, Add a doc comment on the collateral configuration explaining that slash_fraction must be between 0.01 and 1.0 and why: note that values below 1% are disallowed to avoid Decimal precision/rounding to zero and to ensure meaningful slashing amounts; attach this comment near the collateral struct/field definition for slash_fraction (and optionally above the validation block that checks one_percent and self.slash_fraction) so future readers see the constraint and rationale together with the code.crates/basilica-validator/src/collateral/collateral_scan.rs (1)
45-56: Spawned task handle is discarded; shutdown cannot be awaited.The
start()method spawns a background task but does not store theJoinHandle. Whilestop()cancels the token, there's no way to await the task's completion during shutdown. This could lead to incomplete cleanup if the validator shuts down while a sync operation is in progress.Consider storing the
JoinHandleand providing an asyncstopmethod that cancels the token and awaits the task:♻️ Proposed refactor
pub struct Collateral { // ... existing fields ... cancellation_token: CancellationToken, + task_handle: std::sync::Mutex<Option<tokio::task::JoinHandle<()>>>, } - pub fn start(&self) { + pub fn start(&self) { let scanner = self.clone(); - tokio::spawn(async move { + let handle = tokio::spawn(async move { scanner.sync_loop().await; }); + *self.task_handle.lock().unwrap() = Some(handle); } - pub fn stop(&self) { + pub async fn stop(&self) { self.cancellation_token.cancel(); + if let Some(handle) = self.task_handle.lock().unwrap().take() { + let _ = handle.await; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/basilica-validator/src/collateral/collateral_scan.rs` around lines 45 - 56, The start() method currently spawns a tokio task and drops the JoinHandle so shutdown cannot await completion; change the struct to hold an Option<JoinHandle<()>> (behind a Mutex/RwLock if needed) and update start() to store the JoinHandle returned by tokio::spawn; change stop() into an async fn stop(&self) that first calls self.cancellation_token.cancel() and then takes and .awaits the stored JoinHandle (handling JoinError/None appropriately) to ensure sync_loop() completes before shutdown; reference the start(), stop(), sync_loop(), cancellation_token, and the JoinHandle storage when making the changes.crates/basilica-validator/src/service.rs (1)
443-458: DuplicateCollateralEvaluatorinstantiation.A
CollateralEvaluatoris already created at line 312 withininit_collateral_componentsand passed toCollateralManager. Here, a second instance is created for theCollateralscanner. Consider reusing the evaluator frominit_collateral_componentsby passing it throughTaskInputsto avoid inconsistent state if the evaluator ever becomes stateful.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/basilica-validator/src/service.rs` around lines 443 - 458, The code creates a second CollateralEvaluator for the Collateral scanner causing duplicate instances; instead reuse the evaluator constructed in init_collateral_components by threading it through TaskInputs and using that shared Arc<CollateralEvaluator> for both CollateralManager and the Collateral scanner. Update init_collateral_components to attach the evaluator to TaskInputs (e.g., inputs.collateral_evaluator), remove the local Arc::new(CollateralEvaluator::new(...)) in the scanner block, and construct Collateral with the existing inputs.collateral_evaluator so both components share the same evaluator instance.crates/basilica-validator/src/persistence/collateral_persistence.rs (1)
192-206: Inefficient deletion pattern fetches all rows then iterates.The current approach fetches all existing DB rows, then iterates checking against a HashSet. For large collateral sets, this could be slow. Consider using a single
DELETEstatement with a subquery or temporary table, or marking rows with a sync timestamp and deleting stale ones.♻️ Alternative approach using sync timestamp
+ // Delete rows that weren't updated in this sync (stale) + sqlx::query("DELETE FROM collateral_status WHERE updated_at < ?") + .bind(&now) + .execute(&mut *tx) + .await?; - // Delete DB rows that are no longer on-chain (zero-balance nodes) - let db_nodes: Vec<(String, String)> = - sqlx::query_as("SELECT hotkey, node_id FROM collateral_status") - .fetch_all(&mut *tx) - .await?; - - for (hotkey, node_id) in &db_nodes { - if !on_chain_keys.contains(&(hotkey.clone(), node_id.clone())) { - sqlx::query("DELETE FROM collateral_status WHERE hotkey = ? AND node_id = ?") - .bind(hotkey) - .bind(node_id) - .execute(&mut *tx) - .await?; - } - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/basilica-validator/src/persistence/collateral_persistence.rs` around lines 192 - 206, The loop that selects all rows then deletes them one-by-one is inefficient; instead perform a single set-based delete against collateral_status using the on_chain_keys collection: either (A) create a temporary table (or use a table-valued parameter) and bulk insert all (hotkey,node_id) from on_chain_keys then run one DELETE ... WHERE (hotkey,node_id) NOT IN (SELECT hotkey,node_id FROM temp_table) using the same &mut *tx transaction, or (B) add/update a sync_timestamp column for collateral_status, write all on_chain_keys by upsert to set the current timestamp, then execute one DELETE FROM collateral_status WHERE sync_timestamp <> current_sync_ts; reference the variables on_chain_keys, tx and the collateral_status table in collateral_persistence.rs and replace the fetch_all + per-row DELETE logic with the single set-based DELETE approach.crates/basilica-validator/src/rental/mod.rs (1)
567-579: Consider removing unused parameters.The
_gpu_categoryand_gpu_countparameters are not used in the ranking logic. If they're not needed for future extensibility, consider removing them to avoid confusion.♻️ Proposed cleanup
async fn rank_bid_candidates( &self, candidates: Vec<NodeBidCandidate>, - _gpu_category: &str, - _gpu_count: u32, ) -> Vec<NodeBidCandidate> {And update the call site accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/basilica-validator/src/rental/mod.rs` around lines 567 - 579, The rank_bid_candidates function contains unused parameters _gpu_category and _gpu_count; remove these parameters from the async fn rank_bid_candidates signature and update all call sites that invoke rank_bid_candidates (and any trait declarations or impl blocks that define it) to stop passing gpu_category and gpu_count so the function only takes &self and candidates: Vec<NodeBidCandidate>; ensure you also update any trait/interface definitions (if rank_bid_candidates is part of a trait) to keep signatures consistent across implementations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2ed11d3d-9cbd-4854-b224-20f28708be60
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (73)
.gitignoreCargo.tomlcrates/basilica-cli/Cargo.tomlcrates/basilica-cli/src/cli/args.rscrates/basilica-cli/src/cli/commands.rscrates/basilica-cli/src/cli/handlers/collateral.rscrates/basilica-cli/src/cli/handlers/mod.rscrates/basilica-common/examples/compute_node_id.rscrates/basilica-miner/src/main.rscrates/basilica-miner/tests/mod.rscrates/basilica-validator/migrations/017_dual_collateral_event_state.sqlcrates/basilica-validator/migrations/018_collateral_reclaims_index.sqlcrates/basilica-validator/migrations/019_add_collateral_preference_to_miner_nodes.sqlcrates/basilica-validator/src/basilica_api/mod.rscrates/basilica-validator/src/bittensor_core/weight_setter.rscrates/basilica-validator/src/collateral/collateral_scan.rscrates/basilica-validator/src/collateral/evaluator.rscrates/basilica-validator/src/collateral/grace_tracker.rscrates/basilica-validator/src/collateral/manager.rscrates/basilica-validator/src/collateral/mod.rscrates/basilica-validator/src/collateral/slash_executor.rscrates/basilica-validator/src/config/collateral.rscrates/basilica-validator/src/grpc/registration.rscrates/basilica-validator/src/persistence/collateral_grace.rscrates/basilica-validator/src/persistence/collateral_persistence.rscrates/basilica-validator/src/persistence/miner_nodes.rscrates/basilica-validator/src/persistence/mod.rscrates/basilica-validator/src/rental/mod.rscrates/basilica-validator/src/service.rscrates/basilica-validator/tests/collateral_e2e.rscrates/basilica-validator/tests/collateral_slash_flow.rscrates/collateral-contract/CLAUDE.mdcrates/collateral-contract/README.mdcrates/collateral-contract/deploy.shcrates/collateral-contract/flow.shcrates/collateral-contract/foundry.tomlcrates/collateral-contract/query.shcrates/collateral-contract/script/DeployUpgradeable.s.solcrates/collateral-contract/src/Collateral.jsoncrates/collateral-contract/src/Collateral.solcrates/collateral-contract/src/CollateralUpgradableV2ABI.jsoncrates/collateral-contract/src/CollateralUpgradeable.solcrates/collateral-contract/src/CollateralUpgradeableABI.jsoncrates/collateral-contract/src/CollateralUpgradeableV2.solcrates/collateral-contract/src/config.rscrates/collateral-contract/src/lib.rscrates/collateral-contract/src/main.rscrates/collateral-contract/src/tests.rscrates/collateral-contract/test/CollateralBasic.t.solcrates/collateral-contract/test/CollateralTestnet.t.solcrates/collateral-contract/test/CollateralUpgradeable.t.solcrates/collateral-contract/test/IStakingIntegration.t.solcrates/collateral-contract/test/NodeOwnershipEdgeCases.t.solcrates/collateral-contract/test/NodeOwnershipEdgeCases.treecrates/collateral-contract/test/mocks/CollateralUpgradeableUpgradeMock.solcrates/collateral-contract/update_abi.pydocs/collateral-contract.mddocs/miner.mdscripts/collateral/.env.local.examplescripts/collateral/README.mdscripts/collateral/bigint_compare.pyscripts/collateral/e2e-validator-test.shscripts/collateral/integration-test.shscripts/collateral/setup-localnet-env.shscripts/lib/common.shscripts/localnet/configs/validator.example.tomlscripts/localnet/docker-compose.ymlscripts/localnet/init-subnet.shscripts/localnet/register-nodes.shscripts/localnet/start.shscripts/localnet/stop.shscripts/subtensor-local/docker-compose.ymlscripts/subtensor-local/init.py
💤 Files with no reviewable changes (10)
- crates/basilica-validator/src/collateral/mod.rs
- crates/basilica-validator/src/persistence/mod.rs
- crates/collateral-contract/script/DeployUpgradeable.s.sol
- crates/basilica-validator/src/collateral/grace_tracker.rs
- crates/collateral-contract/src/CollateralUpgradableV2ABI.json
- crates/collateral-contract/test/CollateralTestnet.t.sol
- crates/basilica-validator/src/persistence/collateral_grace.rs
- crates/collateral-contract/src/CollateralUpgradeableV2.sol
- crates/collateral-contract/src/Collateral.sol
- crates/collateral-contract/src/Collateral.json
- Remove `deposit_with_config` which was identical to `deposit` with zero callers - Replace `block_in_place`/`block_on` with direct `.await` in `start_rental_inner` since it is already async (H-1 and H-2 from PR review)
- Share CollateralEvaluator via Arc instead of creating two instances - Replace f64 with Decimal in CLI alpha/rao conversions - Lazy-compile regex in extract_gpu_memory_gb using once_cell - Use config warning_threshold_multiplier in warning message - Remove unnecessary async from evaluate() - Clean up unused_imports allow and re-export in collateral/mod.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
crates/basilica-cli/src/cli/handlers/collateral.rs (3)
79-83:⚠️ Potential issue | 🟠 MajorHonor the collateral RPC override here.
resolve_params()still passesNoneas the RPC URL, soBASILICA_COLLATERAL_RPC_URLnever takes effect and local/custom endpoints stay pinned to the built-in network defaults.🛠️ Suggested fix
// Resolve contract address: env var -> built-in default let contract_address = std::env::var("BASILICA_COLLATERAL_CONTRACT_ADDRESS").ok(); + let rpc_url = std::env::var("BASILICA_COLLATERAL_RPC_URL").ok(); - let network_config = CollateralNetworkConfig::from_network(&network, contract_address, None) - .map_err(|e| eyre!("{}", e))?; + let network_config = + CollateralNetworkConfig::from_network(&network, contract_address, rpc_url) + .map_err(|e| eyre!("{}", e))?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/basilica-cli/src/cli/handlers/collateral.rs` around lines 79 - 83, The code calls CollateralNetworkConfig::from_network(&network, contract_address, None) so the BASILICA_COLLATERAL_RPC_URL override is ignored; update the call to pass the resolved RPC URL instead of None by reading std::env::var("BASILICA_COLLATERAL_RPC_URL").ok() (or use the existing resolve_params() result) and pass that Option<String> as the third argument to CollateralNetworkConfig::from_network so custom/local endpoints are honored.
165-176:⚠️ Potential issue | 🟠 MajorKeep transfer and deposit amounts as strings until unit scaling.
Both flows still parse user input as
f64first. By the timeDecimal::try_from(amount)ortao_to_wei(amount)runs, the original decimal string has already been rounded, and large/exponential inputs can overflow or saturate before you ever hit the chain.Also applies to: 270-301, 661-669, 672-766
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/basilica-cli/src/cli/handlers/collateral.rs` around lines 165 - 176, The prompt currently parses user input to f64 (prompt_amount) which loses precision and can overflow; change the prompt to read and validate the raw string instead (e.g., return Result<String, CliError> or add a new prompt_amount_str) by using Input::<String>::with_theme(...).with_prompt(...).validate_with(|input: &String| { try parsing with Decimal::from_str(input) or check regex and ensure >0 }) and only convert to Decimal or wei (Decimal::try_from / tao_to_wei) at the callsites that perform unit scaling; update all usages (the other prompt sites noted around the ranges 270-301, 661-669, 672-766) to accept the string and perform precise conversion later. Ensure error messages remain user-friendly and preserve the original decimal string for scaling.
367-377:⚠️ Potential issue | 🟠 MajorStatus output still hides TAO collateral.
This handler already has access to both assets, but the TAO side is dropped immediately (
let (_tao, alpha)) and the rendered rows only show alpha. Mixed-collateral nodes will be under-reported instatusand reclaim selection output.Also applies to: 387-398, 409-418, 493-502
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/basilica-cli/src/cli/handlers/collateral.rs` around lines 367 - 377, The status output drops TAO collateral because the tuple destructuring uses let (_tao, alpha) and CollateralRow only contains alpha_collateral; update CollateralRow (add a tao_collateral field with a tabled rename like "TAO") and change all destructurings from let (_tao, alpha) to let (tao, alpha) so you capture both values, then populate both tao_collateral and alpha_collateral when building rows in the handlers (the same changes are needed in the other occurrences mentioned around the blocks that previously ignored TAO: 387-398, 409-418, and 493-502). Ensure the table rendering code references the new tao_collateral field so mixed-collateral nodes display both values.crates/basilica-validator/src/persistence/miner_nodes.rs (1)
931-944:⚠️ Potential issue | 🟠 MajorReturn one deterministic GPU summary per node before batching updates.
This query groups by
gua.gpu_name, so a mixed-GPU node produces multipleNodeGpuInforows with the samenode_id.batch_update_collateral_preferences()then overwrites that node several times, and the final preference depends on whichever GPU-name row happens to be processed last.Also applies to: 954-960, 978-983
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/basilica-validator/src/persistence/miner_nodes.rs` around lines 931 - 944, The SQL groups by gua.gpu_name which yields multiple rows per node when a node has mixed GPUs, causing batch_update_collateral_preferences() to overwrite the same node multiple times nondeterministically; fix by making the query return exactly one deterministic GPU summary per node—remove gua.gpu_name from the GROUP BY and replace it with an aggregate (e.g., MAX(gua.gpu_name) AS gpu_name) while keeping COUNT(DISTINCT gua.gpu_uuid) AS gpu_count and grouping only by me.miner_id, me.node_id, m.hotkey (update the query string assigned to query), and apply the same change to the other similar queries used by batch_update_collateral_preferences() so each node_id yields a single row.
🧹 Nitpick comments (1)
crates/basilica-validator/src/rental/mod.rs (1)
567-579: Consider removing unused parameters.The
_gpu_categoryand_gpu_countparameters are unused in the current implementation. If they're not planned for near-term use, consider removing them to reduce API surface noise.However, if these are retained for future collateral-based ranking enhancements (e.g., checking collateral sufficiency per GPU requirements), the current approach is acceptable.
♻️ Optional: Remove unused parameters
async fn rank_bid_candidates( &self, candidates: Vec<NodeBidCandidate>, - _gpu_category: &str, - _gpu_count: u32, ) -> Vec<NodeBidCandidate> {And update the call site at line 477:
- .rank_bid_candidates(candidates, &request.gpu_category, request.gpu_count) + .rank_bid_candidates(candidates)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/basilica-validator/src/rental/mod.rs` around lines 567 - 579, Remove the unused parameters _gpu_category and _gpu_count from the async fn rank_bid_candidates signature in mod.rs and update all related declarations/callers (e.g., the trait/impl that declares rank_bid_candidates and any places that invoke it) so they no longer accept or pass those arguments; ensure function references like rank_bid_candidates and the corresponding trait/method definitions (e.g., the NodeRental/impl) are kept consistent across the codebase.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/basilica-cli/src/cli/handlers/collateral.rs`:
- Around line 399-420: The all-nodes branch ignores params.hotkey so status
--hotkey doesn't filter results; update the code after
collateral_contract::get_all_collaterals(...) and the pending-reclaim handling
(the blocks building Vec<CollateralRow> and the pending-reclaim iteration around
lines ~430-468) to filter `all` (and pending reclaim list) by `params.hotkey`
when Some(hk) — e.g., compare hotkey_to_ss58(&n.miner_hotkey) or the raw
miner_hotkey bytes against params.hotkey (normalize both to the same form)
before mapping to CollateralRow; ensure the same filter logic is applied in both
the "all" collaterals mapping and the pending reclaim mapping so --hotkey
consistently limits displayed rows.
In `@crates/basilica-validator/src/collateral/collateral_scan.rs`:
- Around line 116-121: Nodes, reclaims and last_scanned_block_number are part of
one logical snapshot but are persisted with three separate calls
(sync_all_collateral_nodes, sync_all_reclaims, update_last_scanned_block_number)
which can leave readers with a partial/mixed snapshot on failure; fix this by
performing all three writes inside a single database transaction: add or use a
transactional API on the persistence layer (e.g., persistence.transaction or a
new method like persist_snapshot_transaction) that accepts the nodes, reclaims
and snapshot_block and commits them atomically, and replace the three separate
calls with one transactional call that writes nodes, writes reclaims, and
updates last_scanned_block_number within the same transaction.
- Around line 68-75: The loop currently calls compute_and_store_preferences()
even when self.sync_collateral_state().await fails, allowing preference writes
from stale snapshots; change the control flow in the interval tick handler (the
block containing self.sync_collateral_state() and
compute_and_store_preferences()) so that compute_and_store_preferences() only
runs when sync_collateral_state() returns Ok — i.e., after calling
self.sync_collateral_state().await, if it Errs log the error and skip calling
compute_and_store_preferences() (and similarly update the analogous block later
in the file around compute_and_store_preferences() in the 126-160 region) to
avoid refreshing eligibility from stale DB snapshots.
In `@crates/basilica-validator/src/service.rs`:
- Around line 491-493: The shutdown currently calls
handles.collateral_scanner.stop() which only cancels a token and returns
immediately, risking concurrent DB ops during teardown; change the scanner
lifecycle so shutdown waits for the background task: either modify
Collateral::start() to return and store a JoinHandle (add the handle to
RuntimeHandles) and then on shutdown call .abort() or .await the JoinHandle, or
make Collateral::stop() async and have it await the spawned sync_loop task
completion (join it) before returning; update callers of start()/stop() (e.g.,
where handles.collateral_scanner is created and where it is stopped) to use the
chosen pattern so the scanner's sync_loop is fully finished before proceeding
with persistence teardown.
---
Duplicate comments:
In `@crates/basilica-cli/src/cli/handlers/collateral.rs`:
- Around line 79-83: The code calls
CollateralNetworkConfig::from_network(&network, contract_address, None) so the
BASILICA_COLLATERAL_RPC_URL override is ignored; update the call to pass the
resolved RPC URL instead of None by reading
std::env::var("BASILICA_COLLATERAL_RPC_URL").ok() (or use the existing
resolve_params() result) and pass that Option<String> as the third argument to
CollateralNetworkConfig::from_network so custom/local endpoints are honored.
- Around line 165-176: The prompt currently parses user input to f64
(prompt_amount) which loses precision and can overflow; change the prompt to
read and validate the raw string instead (e.g., return Result<String, CliError>
or add a new prompt_amount_str) by using
Input::<String>::with_theme(...).with_prompt(...).validate_with(|input: &String|
{ try parsing with Decimal::from_str(input) or check regex and ensure >0 }) and
only convert to Decimal or wei (Decimal::try_from / tao_to_wei) at the callsites
that perform unit scaling; update all usages (the other prompt sites noted
around the ranges 270-301, 661-669, 672-766) to accept the string and perform
precise conversion later. Ensure error messages remain user-friendly and
preserve the original decimal string for scaling.
- Around line 367-377: The status output drops TAO collateral because the tuple
destructuring uses let (_tao, alpha) and CollateralRow only contains
alpha_collateral; update CollateralRow (add a tao_collateral field with a tabled
rename like "TAO") and change all destructurings from let (_tao, alpha) to let
(tao, alpha) so you capture both values, then populate both tao_collateral and
alpha_collateral when building rows in the handlers (the same changes are needed
in the other occurrences mentioned around the blocks that previously ignored
TAO: 387-398, 409-418, and 493-502). Ensure the table rendering code references
the new tao_collateral field so mixed-collateral nodes display both values.
In `@crates/basilica-validator/src/persistence/miner_nodes.rs`:
- Around line 931-944: The SQL groups by gua.gpu_name which yields multiple rows
per node when a node has mixed GPUs, causing
batch_update_collateral_preferences() to overwrite the same node multiple times
nondeterministically; fix by making the query return exactly one deterministic
GPU summary per node—remove gua.gpu_name from the GROUP BY and replace it with
an aggregate (e.g., MAX(gua.gpu_name) AS gpu_name) while keeping COUNT(DISTINCT
gua.gpu_uuid) AS gpu_count and grouping only by me.miner_id, me.node_id,
m.hotkey (update the query string assigned to query), and apply the same change
to the other similar queries used by batch_update_collateral_preferences() so
each node_id yields a single row.
---
Nitpick comments:
In `@crates/basilica-validator/src/rental/mod.rs`:
- Around line 567-579: Remove the unused parameters _gpu_category and _gpu_count
from the async fn rank_bid_candidates signature in mod.rs and update all related
declarations/callers (e.g., the trait/impl that declares rank_bid_candidates and
any places that invoke it) so they no longer accept or pass those arguments;
ensure function references like rank_bid_candidates and the corresponding
trait/method definitions (e.g., the NodeRental/impl) are kept consistent across
the codebase.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: afea8583-baa1-4e6b-8531-43b396a81a39
📒 Files selected for processing (12)
PR_372_REVIEW.mdcrates/basilica-cli/src/cli/handlers/collateral.rscrates/basilica-validator/Cargo.tomlcrates/basilica-validator/src/collateral/collateral_scan.rscrates/basilica-validator/src/collateral/evaluator.rscrates/basilica-validator/src/collateral/manager.rscrates/basilica-validator/src/collateral/mod.rscrates/basilica-validator/src/persistence/miner_nodes.rscrates/basilica-validator/src/rental/mod.rscrates/basilica-validator/src/service.rscrates/basilica-validator/tests/collateral_e2e.rscrates/collateral-contract/src/lib.rs
…tworks Remove hardcoded default contract address and deployed block constants. Testnet and Local networks now require an explicit contract address, and scan status initializes from block 0. Also wire through the BASILICA_COLLATERAL_RPC_URL env var in the CLI.
Eliminates floating-point precision loss in tao_to_wei and throughout the deposit/send flows by parsing CLI amounts as strings directly into rust_decimal::Decimal, matching the pattern already used in alpha_to_rao.
The manual test script is no longer needed.
Use separate min_alpha_collateral_increase and asymmetric deposit toggles to catch potential ABI ordering/storage bugs that identical values would mask.
…cking preservation
Replace default placeholder values in deploy.sh with mandatory env vars using bash :? syntax. Suppress asm-keccak256 foundry lint. Rename local test variables to camelCase.
Add upgrade.sh for safe UUPS proxy upgrades with pre-flight checks (role verification, version comparison) and post-upgrade validation. Restructure README deployment section into clear env var tables.
Add missing rules covering initial value declarations, parent contract gaps, parent initializer calls, upgrade mechanism preservation, selfdestruct/delegatecall dangers, ABI compatibility, fork testing, and multisig recommendations.
Simplify `collateral status` to always show all collaterals and reclaims. The single-node query branch and its helpers (require_hotkey, parse_node_id) are removed as they are no longer needed.
Prevent compute_and_store_preferences() from running after a failed sync_collateral_state(), avoiding preference writes derived from stale DB snapshots.
Store the JoinHandle from the collateral scanner's spawned task and await it during shutdown so the sync_loop finishes before persistence teardown begins.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (11)
crates/collateral-contract/foundry.toml (1)
5-6: EVM version split between test and deployment profiles may be unintentional.The default profile uses
evm_version = "cancun"(for tests), while[profile.local]usesevm_version = "london"(for deployment viadeploy.sh). Verification shows the contract does not currently use Cancun-specific opcodes, so there's no immediate compatibility issue. However, the mismatch could be confusing or risky for future maintenance if new code inadvertently relies on Cancun features.Either align the EVM versions to match, or add a comment to
foundry.tomlexplaining why the profiles intentionally target different EVM versions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/collateral-contract/foundry.toml` around lines 5 - 6, The evm_version is inconsistent between the default profile (evm_version = "cancun") and [profile.local] (evm_version = "london") in foundry.toml, which can be confusing; either set both to the same value (e.g., change [profile.local] evm_version to "cancun" or change the default to "london") or add a clear comment in foundry.toml next to the evm_version settings explaining the intentional difference and why deploy.sh targets "london" so future changes won't accidentally rely on Cancun-only opcodes.crates/basilica-validator/src/persistence/simple_persistence.rs (1)
99-101: Note:last_scanned_block_numberis currently write-only.The initialization binds
0i64, but per context snippet 1 (collateral_scan.rs:85-128),sync_collateral_statealways fetches the current finalized block via RPC and never reads the stored value. Theget_last_scanned_block_numbermethod exists but appears unused.This is likely intentional for the RPC-based full-sync architecture, but consider either:
- Documenting this as a placeholder for future incremental sync, or
- Removing the unused storage if full-sync is the permanent design.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/basilica-validator/src/persistence/simple_persistence.rs` around lines 99 - 101, The code inserts a hardcoded 0 into last_scanned_block_number (see simple_persistence::insert of last_scanned_block_number) while sync_collateral_state (in collateral_scan.rs) always reads finalized block from RPC and never calls get_last_scanned_block_number, leaving this field write-only; either document this as an intentional placeholder for future incremental sync (update comments on get_last_scanned_block_number, simple_persistence and collateral_scan.rs to explain why we persist but don’t read) or remove the unused storage and associated methods (remove last_scanned_block_number column handling and get_last_scanned_block_number/insert logic in simple_persistence and any schema migration), and ensure tests and schema migrations are updated accordingly.crates/basilica-validator/src/persistence/collateral_persistence.rs (1)
80-88: Consider using an enum instead of string matching for column selection.The
columnparameter uses string matching which could silently break if column names change. An enum would provide compile-time safety.♻️ Suggested improvement
enum CollateralColumn { Tao, Alpha, } impl CollateralColumn { fn as_str(&self) -> &'static str { match self { Self::Tao => "tao_collateral", Self::Alpha => "alpha_collateral", } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/basilica-validator/src/persistence/collateral_persistence.rs` around lines 80 - 88, Replace the runtime string matching on the `column` parameter with a typed enum to provide compile-time safety: add an enum (e.g., `enum CollateralColumn { Tao, Alpha }`) with a helper like `fn as_str(&self) -> &'static str` that returns `"tao_collateral"` or `"alpha_collateral"`, change the function that currently takes `column: &str` to take `column: CollateralColumn` (or parse the incoming string to this enum at the call site), and then use `column.as_str()` (or match on the enum) to build the `query` instead of matching raw strings and returning the `Unsupported collateral column` error; update any call sites to pass the enum or handle parse errors appropriately.crates/collateral-contract/src/config.rs (1)
34-41: Consider case-insensitive parsing for network names.The
FromStrimplementation only accepts lowercase variants ("mainnet","testnet","local"), but users might reasonably provide"Mainnet"or"MAINNET". This could cause unexpected failures in config files or environment variables.♻️ Suggested fix
fn from_str(s: &str) -> Result<Self, Self::Err> { - match s { + match s.to_lowercase().as_str() { "mainnet" => Ok(Network::Mainnet), "testnet" => Ok(Network::Testnet), "local" => Ok(Network::Local), other => Err(anyhow::anyhow!("Unsupported collateral network: {}", other)), } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/collateral-contract/src/config.rs` around lines 34 - 41, The FromStr implementation for Network (fn from_str) only matches lowercase literals; update it to parse case-insensitively by normalizing the input or using eq_ignore_ascii_case comparisons (e.g., compare s.to_lowercase().as_str() against "mainnet"/"testnet"/"local" or use s.eq_ignore_ascii_case("mainnet") etc.), and keep the existing error path returning the original input on failure (Err(anyhow::anyhow!("Unsupported collateral network: {}", other))) so unexpected-casing inputs like "Mainnet" or "MAINNET" succeed.crates/basilica-cli/src/cli/handlers/collateral.rs (1)
373-400:statuscommand shows all collaterals without hotkey filtering.The
handle_statusfunction fetches and displays all collaterals regardless of the user's wallet. Consider filtering by the caller's EVM address (similar tohandle_reclaimat line 458) to show only relevant entries, or add a--allflag for the full list.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/basilica-cli/src/cli/handlers/collateral.rs` around lines 373 - 400, handle_status currently calls collateral_contract::get_all_collaterals and prints every collateral; change it to either filter results by the caller's EVM address (like handle_reclaim does) or honor a new --all flag on CollateralParams. Specifically: add a boolean flag on CollateralParams (e.g., all_collaterals) or reuse an existing caller evm field, then in handle_status after fetching all (collateral_contract::get_all_collaterals) filter the iterator by matching each collateral's evm/caller field to the wallet/caller address used in handle_reclaim (use the same wallet-to-evm resolution logic as handle_reclaim) when the flag is false, and only skip filtering when the new --all flag is true; update the CLI param parsing accordingly so the default remains filtered by caller.crates/collateral-contract/test/CollateralBasic.t.sol (2)
430-491: Storage slot manipulation test is thorough but fragile.This test validates an important fix (alpha-only reclaim denial), but relies on hardcoded storage slot indices (lines 459, 473). The inline comments documenting the layout are helpful, but consider adding a comment noting this test needs updating if the contract storage layout changes, or adding a runtime assertion to verify the slot calculation.
The test logic is correct: it mutates a TAO reclaim into an alpha-only reclaim and verifies that
denyReclaimRequestsucceeds (the fix changedamount == 0toamount == 0 && alphaAmount == 0).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/collateral-contract/test/CollateralBasic.t.sol` around lines 430 - 491, The test testDenyAlphaOnlyReclaim uses hardcoded storage-slot math (reclaimsSlot, baseSlot, amountSlot, alphaAmountSlot) which is fragile; add a runtime assertion that the computed slots actually map to the struct fields by comparing vm.load(address(collateral), amountSlot) and vm.load(address(collateral), alphaAmountSlot) to the values read from collateral.reclaims(0) before mutating them, and add a short comment above the slot calculations warning that these indices must be updated if the contract storage layout changes (or replace with a helper that computes the keccak slots consistently).
1278-1290: Upgrade test does not verify persistence of new state variables.The test verifies the upgrade mechanism works, but the
CollateralUpgradeableUpgradeMock.initializeUpgradeMock()is empty (see context snippet 2). This means the test doesn't verify thattaoDepositsEnabledandalphaDepositsEnabledpersist correctly across upgrades.Consider adding assertions to verify state persistence:
💡 Suggested enhancement
function testUpgrade() public { + // Verify pre-upgrade state + assertTrue(collateral.taoDepositsEnabled()); + assertTrue(collateral.alphaDepositsEnabled()); + CollateralUpgradeableUpgradeMock newImplementation = new CollateralUpgradeableUpgradeMock(); vm.expectEmit(true, true, false, false, address(collateral)); emit ContractUpgraded(2, address(newImplementation)); vm.prank(ADMIN); collateral.upgradeToAndCall(address(newImplementation), ""); assertEq(collateral.getVersion(), 2); + + // Verify state persists after upgrade + assertTrue(collateral.taoDepositsEnabled()); + assertTrue(collateral.alphaDepositsEnabled()); + assertEq(collateral.netuid(), NETUID); + assertEq(collateral.trustee(), TRUSTEE); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/collateral-contract/test/CollateralBasic.t.sol` around lines 1278 - 1290, The testUpgrade currently upgrades to CollateralUpgradeableUpgradeMock but never verifies persistence of new storage vars; modify the test to (1) set known pre-upgrade values for taoDepositsEnabled and alphaDepositsEnabled on the original collateral, (2) encode a call to CollateralUpgradeableUpgradeMock.initializeUpgradeMock (or implement that function to set the expected post-upgrade values) and pass that calldata into collateral.upgradeToAndCall(address(newImplementation), initCalldata) under vm.prank(ADMIN), and (3) assert collateral.taoDepositsEnabled() and collateral.alphaDepositsEnabled() both before and after the upgrade and assert getVersion() == 2 after upgrade. Ensure you reference the initializeUpgradeMock function on CollateralUpgradeableUpgradeMock and the collateral.upgradeToAndCall and collateral.getVersion methods when making these changes.crates/collateral-contract/test/NodeOwnershipEdgeCases.t.sol (2)
793-799: Non-deterministicgetStakemock could cause flaky tests if used.The mock returns
type(uint256).max - gasleft()which varies based on execution context. While current tests don't appear to use this, future tests relying ongetStakewill have non-deterministic behavior. Consider returning a fixed value or making it configurable.💡 Suggested fix for deterministic behavior
contract _StakingV2Mock { + uint256 public stakeReturnValue = 1000 ether; + function transferStake(bytes32, bytes32, uint256, uint256, uint256) external payable {} function moveStake(bytes32, bytes32, uint256, uint256, uint256) external payable {} function getStake(bytes32, bytes32, uint256) external view returns (uint256) { - return type(uint256).max - gasleft(); + return stakeReturnValue; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/collateral-contract/test/NodeOwnershipEdgeCases.t.sol` around lines 793 - 799, The getStake implementation in _StakingV2Mock is non-deterministic (returns type(uint256).max - gasleft()); change it to return a deterministic value by replacing that expression with a fixed constant or a configurable state variable (e.g., a uint256 stakeValue with a setter) so tests behave predictably; update the _StakingV2Mock contract to include the constant or state + setter and have getStake(bytes32, bytes32, uint256) return that deterministic stakeValue.
94-104: Storage slot assumption is fragile and may break on contract layout changes.The helper
_nodeToMinerSlothardcodes slot 5 fornodeToMiner. While this works now, any reordering of state variables inCollateralUpgradeablewill silently break these tests without compile-time errors. Consider adding a comment documenting the expected layout or adding a sanity check assertion insetUp().💡 Suggested sanity check
function setUp() public { // ... existing setup ... + // Sanity check: verify slot assumption by depositing and checking storage + // If this fails, the storage layout has changed and _nodeToMinerSlot needs updating + vm.prank(ALICE); + collateral.deposit{value: MIN_DEPOSIT}(HOTKEY_1, EXECUTOR_ID_1, ALPHA_HOTKEY, 0); + assertEq( + address(uint160(uint256(vm.load(address(collateral), _nodeToMinerSlot(HOTKEY_1, EXECUTOR_ID_1))))), + ALICE, + "Storage slot assumption for nodeToMiner is incorrect" + ); + // Reset state for tests - or adjust setUp to account for this }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/collateral-contract/test/NodeOwnershipEdgeCases.t.sol` around lines 94 - 104, The test helper hardcodes slot 5 in _nodeToMinerSlot which is fragile; add a sanity check in setUp() that verifies the hardcoded slot matches the contract's actual mapping layout by using _seedNodeOwner(hotkey, nodeId, owner) with a sentinel address and then reading back via the contract's accessor (e.g., call the CollateralUpgradeable mapping/getter for nodeToMiner using the same hotkey/nodeId) to assert the stored owner equals the sentinel; if the read value mismatches, fail the setup with a clear message so future contract layout changes surface immediately.crates/collateral-contract/src/tests.rs (2)
17-18: Differentiate the validator hotkey from the deposit hotkey in this test.The initializer field here is the contract-wide validator hotkey, but the test later reuses the same
[2u8; 32]value as the per-deposit alpha hotkey. With identical values, a wiring mix-up between those two paths would still pass. Use distinct names/values and assert the stored validator hotkey explicitly.Also applies to: 86-87, 94-105, 116-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/collateral-contract/src/tests.rs` around lines 17 - 18, The test reuses the same bytes value for the contract-wide validator hotkey (initializer parameter passed to initialize) and the per-deposit alpha hotkey, which masks wiring bugs; change the test in tests.rs to use distinct byte values/names (e.g., validator_hotkey_vs deposit_alpha_hotkey) when calling initialize and when creating deposits, then add an explicit assertion that the contract-stored validator hotkey (the initializer field passed to initialize) equals the validator_hotkey value you set; update all affected blocks mentioned (around the initialize call and deposit creation at the referenced locations) to use these distinct symbols and assertions.
121-155: Exercise the disabled alpha path, not just the stored flag.This test proves
alphaDepositsEnabledwas initialized tofalse, but it never attempts a non-zero alpha deposit under that configuration. A regression indeposit()that ignores the toggle would still pass here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/collateral-contract/src/tests.rs` around lines 121 - 155, The test currently only checks the alphaDepositsEnabled flag but never exercises the disabled path; add a call to proxied.deposit using a non-zero alpha deposit (e.g., FixedBytes::from_slice(&alpha_hotkey) with a non-zero value such as min_alpha_collateral_increase or similar) while alphaDepositsEnabled is false and assert that the call fails/reverts (expect an error/unwrap_err or revert assertion) to ensure deposit() enforces the toggle; reference proxied.deposit, alphaDepositsEnabled, alpha_hotkey, and min_alpha_collateral_increase when adding this failing-case assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/basilica-cli/src/cli/handlers/collateral.rs`:
- Around line 138-141: The wei_to_tao function currently casts U256 -> u128 ->
f64 losing precision for large balances; change it to convert the U256 to a
Decimal (using the same Decimal type used for alpha conversions), perform the
division by Decimal::from_str("1e18") or Decimal::new accordingly, and return a
Decimal (or string) instead of f64; then update any callers/format strings that
used wei_to_tao (e.g., formatted with "{:.4}") to format the Decimal
consistently (matching alpha formatting) so precision is preserved for large TAO
balances.
In `@crates/basilica-validator/src/collateral/collateral_scan.rs`:
- Around line 199-213: The current use of unwrap_or_else on
persistence.get_alpha_collateral_amount masks DB errors and converts them into a
zero collateral value; change this so that failures from
get_alpha_collateral_amount are distinguished from a genuine “no collateral”
None: instead of swallowing the error in unwrap_or_else, propagate the error
(return Err) or convert it into a sentinel (e.g., collateral_lookup_failed) by
returning a Result<Option<U256>, _> up from this function, then handle that case
where collateral_alpha is computed (the u256_to_alpha call) so transient DB
failures do not default to Decimal::ZERO and cause incorrect Fallback decisions.
Ensure you update the call sites and signatures that use
get_alpha_collateral_amount/persistence and adjust the collateral_alpha logic to
branch on lookup error vs Some(None).
In `@crates/collateral-contract/README.md`:
- Around line 229-235: Replace the real-looking private key literal used for the
PRIVATE_KEY example in the README with an obviously fake placeholder (e.g.,
0x<private-key> or 0xDEADBEEF...), so update the export line that sets
PRIVATE_KEY in the README example and any matching usage in the adjacent
collateral-cli invocation; ensure the placeholder clearly cannot be mistaken for
a real secret to avoid gitleaks alerts while keeping the example shape the same.
In `@crates/collateral-contract/upgrade.sh`:
- Around line 11-13: The script currently prints the full RPC_URL which may
contain embedded secrets; update the upgrade.sh print logic to redact or replace
sensitive parts of RPC_URL before echoing (e.g., strip credentials and API keys
or derive and echo only the host) — locate the echo statements referencing the
RPC_URL variable (the lines with echo "RPC URL: $RPC_URL") and change them
to use a redacted variable (compute a safe_rpc_url from RPC_URL by removing
userinfo/query params or only extracting the host) and print that instead.
- Around line 58-62: The script fails when invoked from outside its directory
because the forge command uses a relative path; modify upgrade.sh so it first
changes to the script directory (or runs the forge command via a subshell that
cds into the script directory) before running the FOUNDRY_PROFILE=local forge
create src/CollateralUpgradeable.sol:CollateralUpgradeable --rpc-url "$RPC_URL"
--private-key "$PRIVATE_KEY" --legacy --broadcast command; ensure you reference
the script's directory using a robust construct (e.g., dirname "$0") so
CollateralUpgradeable is resolved correctly when the script is run from the repo
root or CI.
---
Nitpick comments:
In `@crates/basilica-cli/src/cli/handlers/collateral.rs`:
- Around line 373-400: handle_status currently calls
collateral_contract::get_all_collaterals and prints every collateral; change it
to either filter results by the caller's EVM address (like handle_reclaim does)
or honor a new --all flag on CollateralParams. Specifically: add a boolean flag
on CollateralParams (e.g., all_collaterals) or reuse an existing caller evm
field, then in handle_status after fetching all
(collateral_contract::get_all_collaterals) filter the iterator by matching each
collateral's evm/caller field to the wallet/caller address used in
handle_reclaim (use the same wallet-to-evm resolution logic as handle_reclaim)
when the flag is false, and only skip filtering when the new --all flag is true;
update the CLI param parsing accordingly so the default remains filtered by
caller.
In `@crates/basilica-validator/src/persistence/collateral_persistence.rs`:
- Around line 80-88: Replace the runtime string matching on the `column`
parameter with a typed enum to provide compile-time safety: add an enum (e.g.,
`enum CollateralColumn { Tao, Alpha }`) with a helper like `fn as_str(&self) ->
&'static str` that returns `"tao_collateral"` or `"alpha_collateral"`, change
the function that currently takes `column: &str` to take `column:
CollateralColumn` (or parse the incoming string to this enum at the call site),
and then use `column.as_str()` (or match on the enum) to build the `query`
instead of matching raw strings and returning the `Unsupported collateral
column` error; update any call sites to pass the enum or handle parse errors
appropriately.
In `@crates/basilica-validator/src/persistence/simple_persistence.rs`:
- Around line 99-101: The code inserts a hardcoded 0 into
last_scanned_block_number (see simple_persistence::insert of
last_scanned_block_number) while sync_collateral_state (in collateral_scan.rs)
always reads finalized block from RPC and never calls
get_last_scanned_block_number, leaving this field write-only; either document
this as an intentional placeholder for future incremental sync (update comments
on get_last_scanned_block_number, simple_persistence and collateral_scan.rs to
explain why we persist but don’t read) or remove the unused storage and
associated methods (remove last_scanned_block_number column handling and
get_last_scanned_block_number/insert logic in simple_persistence and any schema
migration), and ensure tests and schema migrations are updated accordingly.
In `@crates/collateral-contract/foundry.toml`:
- Around line 5-6: The evm_version is inconsistent between the default profile
(evm_version = "cancun") and [profile.local] (evm_version = "london") in
foundry.toml, which can be confusing; either set both to the same value (e.g.,
change [profile.local] evm_version to "cancun" or change the default to
"london") or add a clear comment in foundry.toml next to the evm_version
settings explaining the intentional difference and why deploy.sh targets
"london" so future changes won't accidentally rely on Cancun-only opcodes.
In `@crates/collateral-contract/src/config.rs`:
- Around line 34-41: The FromStr implementation for Network (fn from_str) only
matches lowercase literals; update it to parse case-insensitively by normalizing
the input or using eq_ignore_ascii_case comparisons (e.g., compare
s.to_lowercase().as_str() against "mainnet"/"testnet"/"local" or use
s.eq_ignore_ascii_case("mainnet") etc.), and keep the existing error path
returning the original input on failure (Err(anyhow::anyhow!("Unsupported
collateral network: {}", other))) so unexpected-casing inputs like "Mainnet" or
"MAINNET" succeed.
In `@crates/collateral-contract/src/tests.rs`:
- Around line 17-18: The test reuses the same bytes value for the contract-wide
validator hotkey (initializer parameter passed to initialize) and the
per-deposit alpha hotkey, which masks wiring bugs; change the test in tests.rs
to use distinct byte values/names (e.g., validator_hotkey_vs
deposit_alpha_hotkey) when calling initialize and when creating deposits, then
add an explicit assertion that the contract-stored validator hotkey (the
initializer field passed to initialize) equals the validator_hotkey value you
set; update all affected blocks mentioned (around the initialize call and
deposit creation at the referenced locations) to use these distinct symbols and
assertions.
- Around line 121-155: The test currently only checks the alphaDepositsEnabled
flag but never exercises the disabled path; add a call to proxied.deposit using
a non-zero alpha deposit (e.g., FixedBytes::from_slice(&alpha_hotkey) with a
non-zero value such as min_alpha_collateral_increase or similar) while
alphaDepositsEnabled is false and assert that the call fails/reverts (expect an
error/unwrap_err or revert assertion) to ensure deposit() enforces the toggle;
reference proxied.deposit, alphaDepositsEnabled, alpha_hotkey, and
min_alpha_collateral_increase when adding this failing-case assertion.
In `@crates/collateral-contract/test/CollateralBasic.t.sol`:
- Around line 430-491: The test testDenyAlphaOnlyReclaim uses hardcoded
storage-slot math (reclaimsSlot, baseSlot, amountSlot, alphaAmountSlot) which is
fragile; add a runtime assertion that the computed slots actually map to the
struct fields by comparing vm.load(address(collateral), amountSlot) and
vm.load(address(collateral), alphaAmountSlot) to the values read from
collateral.reclaims(0) before mutating them, and add a short comment above the
slot calculations warning that these indices must be updated if the contract
storage layout changes (or replace with a helper that computes the keccak slots
consistently).
- Around line 1278-1290: The testUpgrade currently upgrades to
CollateralUpgradeableUpgradeMock but never verifies persistence of new storage
vars; modify the test to (1) set known pre-upgrade values for taoDepositsEnabled
and alphaDepositsEnabled on the original collateral, (2) encode a call to
CollateralUpgradeableUpgradeMock.initializeUpgradeMock (or implement that
function to set the expected post-upgrade values) and pass that calldata into
collateral.upgradeToAndCall(address(newImplementation), initCalldata) under
vm.prank(ADMIN), and (3) assert collateral.taoDepositsEnabled() and
collateral.alphaDepositsEnabled() both before and after the upgrade and assert
getVersion() == 2 after upgrade. Ensure you reference the initializeUpgradeMock
function on CollateralUpgradeableUpgradeMock and the collateral.upgradeToAndCall
and collateral.getVersion methods when making these changes.
In `@crates/collateral-contract/test/NodeOwnershipEdgeCases.t.sol`:
- Around line 793-799: The getStake implementation in _StakingV2Mock is
non-deterministic (returns type(uint256).max - gasleft()); change it to return a
deterministic value by replacing that expression with a fixed constant or a
configurable state variable (e.g., a uint256 stakeValue with a setter) so tests
behave predictably; update the _StakingV2Mock contract to include the constant
or state + setter and have getStake(bytes32, bytes32, uint256) return that
deterministic stakeValue.
- Around line 94-104: The test helper hardcodes slot 5 in _nodeToMinerSlot which
is fragile; add a sanity check in setUp() that verifies the hardcoded slot
matches the contract's actual mapping layout by using _seedNodeOwner(hotkey,
nodeId, owner) with a sentinel address and then reading back via the contract's
accessor (e.g., call the CollateralUpgradeable mapping/getter for nodeToMiner
using the same hotkey/nodeId) to assert the stored owner equals the sentinel; if
the read value mismatches, fail the setup with a clear message so future
contract layout changes surface immediately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c3d80515-bda5-4619-b6d6-bd2f95cc0c56
📒 Files selected for processing (17)
crates/basilica-cli/src/cli/commands.rscrates/basilica-cli/src/cli/handlers/collateral.rscrates/basilica-validator/src/collateral/collateral_scan.rscrates/basilica-validator/src/persistence/collateral_persistence.rscrates/basilica-validator/src/persistence/simple_persistence.rscrates/basilica-validator/src/service.rscrates/collateral-contract/README.mdcrates/collateral-contract/deploy.shcrates/collateral-contract/flow.shcrates/collateral-contract/foundry.tomlcrates/collateral-contract/src/config.rscrates/collateral-contract/src/tests.rscrates/collateral-contract/test/CollateralBasic.t.solcrates/collateral-contract/test/NodeOwnershipEdgeCases.t.solcrates/collateral-contract/test/NodeOwnershipEdgeCases.treecrates/collateral-contract/upgrade.shdocs/collateral-contract.md
💤 Files with no reviewable changes (1)
- crates/collateral-contract/flow.sh
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/collateral-contract/README.md`:
- Around line 364-377: Reorder and edit the examples so the README no longer
leads with unbounded full-chain scans; replace the first examples of
"collateral-cli --network testnet events scan" with bounded scans using the
--from-block and --to-block flags (e.g., recent range or specific block window)
and keep the provided "last 100 blocks" example as the primary pattern; add a
short note describing chunked backfills for long ranges (recommend splitting
large ranges into smaller windows and iterating with --from-block/--to-block to
avoid oversized eth_getLogs requests), and ensure the custom-contract and
JSON-format examples still show bounded scans or explicitly call out that
unbounded/default full-range scans may fail on real RPC providers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 101993c5-ab56-4788-8dd9-3fd6d14d7ffb
📒 Files selected for processing (1)
crates/collateral-contract/README.md
The deploy script defaulted to 1 second, which is only useful for testing. Localnet already overrides this via setup-localnet-env.sh, so the deploy.sh default should be production-safe (86400s = 1 day).
Only alpha token collateral is supported for now. TAO deposits can still be enabled by setting TAO_DEPOSITS_ENABLED=true.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/collateral-contract/deploy.sh (1)
15-19: Let callers choose the Foundry profile.Line 15 and Line 44 force
FOUNDRY_PROFILE=local, so a non-local deploy still compiles with the local profile even when the caller exports a different one. That is brittle for a generic deployment script.Suggested fix
+FOUNDRY_PROFILE="${FOUNDRY_PROFILE:-default}" + -impl_output="$(FOUNDRY_PROFILE=local forge create src/CollateralUpgradeable.sol:CollateralUpgradeable \ +impl_output="$(FOUNDRY_PROFILE="$FOUNDRY_PROFILE" forge create src/CollateralUpgradeable.sol:CollateralUpgradeable \ --rpc-url "$RPC_URL" \ --private-key "$PRIVATE_KEY" \ --legacy \ --broadcast)" @@ -proxy_output="$(FOUNDRY_PROFILE=local forge create lib/openzeppelin-contracts/contracts/proxy/ERC1967/ERC1967Proxy.sol:ERC1967Proxy \ +proxy_output="$(FOUNDRY_PROFILE="$FOUNDRY_PROFILE" forge create lib/openzeppelin-contracts/contracts/proxy/ERC1967/ERC1967Proxy.sol:ERC1967Proxy \ --rpc-url "$RPC_URL" \ --private-key "$PRIVATE_KEY" \ --legacy \ --broadcast \ --constructor-args "$IMPLEMENTATION_ADDRESS" "$INIT_DATA")"Also applies to: 44-49
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/collateral-contract/deploy.sh` around lines 15 - 19, The script forces FOUNDRY_PROFILE=local inline before the forge commands (e.g., the impl_output assignment that runs `forge create src/CollateralUpgradeable.sol:CollateralUpgradeable`) which prevents callers from supplying a different profile; remove the inline `FOUNDRY_PROFILE=local` prefix (or replace it with a default like `FOUNDRY_PROFILE="${FOUNDRY_PROFILE:-local}"` at the top of the script) so the forge invocations respect the environment-provided FOUNDRY_PROFILE, and apply the same change to the second forge invocation block (the verify/broadcast section around lines 44-49) so both impl_output and the later verify/broadcast commands use the caller-specified profile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/collateral-contract/deploy.sh`:
- Around line 1-12: The deploy.sh script fails when run from repo root because
it uses project-relative forge paths (e.g.,
"src/CollateralUpgradeable.sol:CollateralUpgradeable" and
"lib/openzeppelin-contracts/contracts/proxy/ERC1967/ERC1967Proxy.sol:ERC1967Proxy")
but never switches to the Foundry project directory; fix by adding a defensive
directory change near the top of deploy.sh (before any forge invocation) such as
changing into the script's directory (use dirname "$0" or similar) and erroring
out if the cd fails so all subsequent forge commands resolve foundry.toml and
the relative contract paths correctly.
In `@crates/collateral-contract/README.md`:
- Line 256: Replace the duplicated MD5-like digest used with the
--url-content-sha256 example in the README: the value
"d41d8cd98f00b204e9800998ecf8427ed41d8cd98f00b204e9800998ecf8427e" is not a
valid SHA-256; update the example to a genuine 64-hex SHA-256 digest (or
explicitly mark it as a placeholder like "<SHA256_HEX_PLACEHOLDER>") and make
the same change for the other two occurrences of this bogus digest in the file
so the README shows valid SHA-256 examples for the --url-content-sha256 flag.
---
Nitpick comments:
In `@crates/collateral-contract/deploy.sh`:
- Around line 15-19: The script forces FOUNDRY_PROFILE=local inline before the
forge commands (e.g., the impl_output assignment that runs `forge create
src/CollateralUpgradeable.sol:CollateralUpgradeable`) which prevents callers
from supplying a different profile; remove the inline `FOUNDRY_PROFILE=local`
prefix (or replace it with a default like
`FOUNDRY_PROFILE="${FOUNDRY_PROFILE:-local}"` at the top of the script) so the
forge invocations respect the environment-provided FOUNDRY_PROFILE, and apply
the same change to the second forge invocation block (the verify/broadcast
section around lines 44-49) so both impl_output and the later verify/broadcast
commands use the caller-specified profile.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fc096cd0-da52-41db-84f1-8d80a6d361c8
📒 Files selected for processing (2)
crates/collateral-contract/README.mdcrates/collateral-contract/deploy.sh
| --node-id 9e0a4d34-3110-48d1-b3c5-580f44270f13 \ | ||
| --url "https://proof-server.testnet.com/evidence/456" \ | ||
| --url-content-md5-checksum d41d8cd98f00b204e9800998ecf8427e | ||
| --url-content-sha256 d41d8cd98f00b204e9800998ecf8427ed41d8cd98f00b204e9800998ecf8427e |
Contributor
There was a problem hiding this comment.
Use actual SHA-256 examples here.
These values are duplicated MD5 digests, not realistic SHA-256 examples. That undercuts the README’s SHA-256 migration guidance and makes copy/paste validation harder. Replace them with genuine 64-hex SHA-256 digests or label them as placeholders.
Suggested doc fix
- --url-content-sha256 d41d8cd98f00b204e9800998ecf8427ed41d8cd98f00b204e9800998ecf8427e
+ --url-content-sha256 e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855
...
- --url-content-sha256 5d41402abc4b2a76b9719d911017c5925d41402abc4b2a76b9719d911017c592
+ --url-content-sha256 2cf24dba5fb0a30e26e83b2ac5b9e29e1b161e5c1fa7425e73043362938b9824
...
- --url-content-sha256 098f6bcd4621d373cade4e832627b4f6098f6bcd4621d373cade4e832627b4f6
+ --url-content-sha256 9f86d081884c7d659a2feaa0c55ad015a3bf4f1b2b0b822cd15d6c15b0f00a08Also applies to: 281-281, 303-303
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/collateral-contract/README.md` at line 256, Replace the duplicated
MD5-like digest used with the --url-content-sha256 example in the README: the
value "d41d8cd98f00b204e9800998ecf8427ed41d8cd98f00b204e9800998ecf8427e" is not
a valid SHA-256; update the example to a genuine 64-hex SHA-256 digest (or
explicitly mark it as a placeholder like "<SHA256_HEX_PLACEHOLDER>") and make
the same change for the other two occurrences of this bogus digest in the file
so the README shows valid SHA-256 examples for the --url-content-sha256 flag.
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
2 participants
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
This PR hardens and completes Basilica’s collateral integration across:
Primary outcomes are stronger reclaim/slash invariants, correct dual-asset state handling (TAO + alpha), and end-to-end SHA-256 evidence consistency.
What Changed
1) Contract hardening and lifecycle correctness (
crates/collateral-contract)CollateralUpgradeablefor deposit, reclaim, deny, and slash flows.taoDepositsEnabledandalphaDepositsEnabledtoggles.minAlphaCollateralIncrease.bytes32) across contract events and calls.collaterals->taoCollaterals,CONTRACT_HOTKEY->validatorHotkey).2) Validator integration and state consistency (
crates/basilica-validator)Deposit,ReclaimProcessStarted,Denied,Reclaimed,Slashed).rpc_urloverride support for validator configuration.3) CLI/bindings and API surface alignment (
collateral-cli+ Rust library)4) Localnet collateral workflow improvements (
scripts/*)Behavioral/API Changes to Validate
bytes32) end-to-end.rpc_urloverride.Summary by CodeRabbit
New Features
Improvements