Conversation
WalkthroughRename Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Trustee as Trustee/Admin
participant CLI as collateral-cli
participant Lib as collateral_contract::lib
participant Contract as CollateralUpgradeable
participant Event as SlashedEvent
participant Validator as basilica-validator
participant DB as CollateralDB
Trustee->>CLI: tx slash-collateral --slash-amount X
CLI->>Lib: slash_collateral(hotkey, executorId, X, url, md5)
Lib->>Contract: slashCollateral(hotkey, executorId, slashAmount=X, url, md5)
alt slashAmount <= collateral
Contract->>Contract: collateral[executor] -= X
Contract-->>Event: emit Slashed(..., slashAmount=X, ...)
Event-->>Validator: Slashed(hotkey, executorId, slashAmount=X, url, md5)
Validator->>DB: UPDATE collateral = collateral - X
Contract-->>CLI: Tx receipt
else slashAmount > collateral
Contract-->>CLI: revert InsufficientCollateralForSlash
CLI-->>Trustee: error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
crates/collateral-contract/flow.sh (1)
39-49: Flow will fail: hardcoded reclaim_request_id, finalize+deny on same request, and slashing after collateral becomes 0.
- You finalize and then deny the same reclaim (likely revert).
- You assume reclaim_request_id=0 (not guaranteed).
- You slash 10 after setting collateral to 0, so the tx should revert (InsufficientCollateralForSlash), aborting the script under set -e.
Proposed minimal fix: re-deposit before slashing so the slash path is exercised reliably.
@@ -# check the collaterals should be 0 -collateral-cli --network "$NETWORK" --contract-address "$CONTRACT_ADDRESS" query collaterals \ ---hotkey "$HOTKEY" \ ---executor-id "$EXECUTOR_ID" +# check the collaterals should be 0 +collateral-cli --network "$NETWORK" --contract-address "$CONTRACT_ADDRESS" query collaterals \ +--hotkey "$HOTKEY" \ +--executor-id "$EXECUTOR_ID" + +# re-deposit so slash has balance to act on +collateral-cli --network "$NETWORK" --contract-address "$CONTRACT_ADDRESS" tx deposit \ +--private-key "$PRIVATE_KEY" \ +--hotkey "$HOTKEY" \ +--amount 10 \ +--executor-id "$EXECUTOR_ID" @@ # slash the collateral collateral-cli --network "$NETWORK" --contract-address "$CONTRACT_ADDRESS" tx slash-collateral \ --private-key "$PRIVATE_KEY" \ --hotkey "$HOTKEY" \ --executor-id "$EXECUTOR_ID" \ --slash-amount 10 \ --url https://www.tplr.ai/ \ --url-content-md5-checksum 269ff519d1140a175941ea4b00ccbe0dOptional: gate finalize/deny behind a RECLAIM_REQUEST_ID env var to avoid racing unknown IDs. I can draft that if you want.
Also applies to: 51-58, 60-67
crates/basilica-validator/src/persistence/collateral_persistence.rs (2)
178-199: Bug: 256-bit arithmetic done inside SQLite will lose precision; compute in Rust. Also store event miner, not zero.
- collateral is persisted as decimal TEXT; SQL
collateral = collateral - ?coerces to 64-bit numeric/float → precision loss/corruption.- The Slashed event provides
miner; writing ZERO wipes data.Fix by computing in Rust with U256 and updating the exact value; bind
slashed.miner.- let query = "UPDATE collateral_status SET collateral = collateral - ?, miner = ? , url = ? , url_content_md5_checksum = ?, updated_at = ? WHERE id = ?"; - if slashed.slashAmount > collateral { - error!( - "Slashed amount {} is greater than collateral {} in database", - slashed.slashAmount, collateral - ); - return Err(anyhow::anyhow!("Slashed amount is greater than collateral")); - } - - sqlx::query(query) - .bind(slashed.slashAmount.to_string()) - .bind(format!( - "0x{}", - Address::ZERO.as_slice().encode_hex::<String>() - )) + let query = "UPDATE collateral_status SET collateral = ?, miner = ? , url = ? , url_content_md5_checksum = ?, updated_at = ? WHERE id = ?"; + // Compute precisely using U256 to avoid DB numeric coercion. + let new_collateral = collateral.saturating_sub(slashed.slashAmount); + if slashed.slashAmount > collateral { + error!( + "Slashed amount {} is greater than collateral {} in database", + slashed.slashAmount, collateral + ); + return Err(anyhow::anyhow!("Slashed amount is greater than collateral")); + } + + sqlx::query(query) + .bind(new_collateral.to_string()) + .bind(format!( + "0x{}", + slashed.miner.as_slice().encode_hex::<String>() + )) .bind(slashed.url.clone()) .bind(slashed.urlContentMd5Checksum.encode_hex::<String>()) .bind(now) .bind(id) .execute(self.pool()) .await?;
178-199: Refactor DB-side collateral arithmetic — single occurrence found
Search found one SQL-side arithmetic oncollateralat crates/basilica-validator/src/persistence/collateral_persistence.rs:178 — compute the new collateral in Rust and UPDATEcollateral = ?(bind the computed value) instead ofcollateral = collateral - ?.crates/collateral-contract/src/CollateralUpgradeable.sol (2)
328-358: Add zero-amount guard and clarify burn sink
- Slashing with
slashAmount == 0is a no-op but still emits events. Guard it to avoid noisy/ambiguous transactions.- Burning to
address(0)is irreversible and atypical for ETH; consider a configurable sink (e.g., treasury) for transparency/auditing, or document the intent explicitly.Apply this minimal fix for zero-amount guard:
uint256 slashAmount, ... - if (amount == 0) { + if (amount == 0) { revert AmountZero(); } + if (slashAmount == 0) { + revert AmountZero(); + } + if (slashAmount > amount) { revert InsufficientCollateralForSlash(); }Optional: route slashed funds to a configurable sink.
- // burn the collateral - (bool success, ) = payable(address(0)).call{value: slashAmount}(""); + // send slashed funds to sink (e.g., treasury); defaults to zero address if unset + address sink = address(0); // TODO: make configurable via admin setter + (bool success, ) = payable(sink).call{value: slashAmount}("");
409-413: Upgrade event versioning is misleading; always emits “2” today
this.getVersion() + 1calls the current implementation (via the proxy) and will always decode to the old version, making the event incorrect for future upgrades. Either drop the computed version or read it fromnewImplementationviastaticcall.Apply this diff:
- function _authorizeUpgrade( - address newImplementation - ) internal override onlyRole(UPGRADER_ROLE) { - emit ContractUpgraded(this.getVersion() + 1, newImplementation); - } + function _authorizeUpgrade( + address newImplementation + ) internal override onlyRole(UPGRADER_ROLE) { + uint256 newVersion = 0; + (bool ok, bytes memory data) = + newImplementation.staticcall(abi.encodeWithSignature("getVersion()")); + if (ok && data.length >= 32) { + newVersion = abi.decode(data, (uint256)); + } + emit ContractUpgraded(newVersion, newImplementation); + }
🧹 Nitpick comments (8)
crates/collateral-contract/README.md (1)
198-200: Docs: New --slash-amount flag is aligned with CLI; clarify units.Looks good. Minor phrasing tweak for clarity: explicitly say “in RAO” in the examples/comments to match the CLI help text.
Also applies to: 207-209
crates/collateral-contract/src/main.rs (3)
60-63: Grammar nit: clarify unit phrasing.- /// Amount to deposit RAO + /// Amount to deposit in RAO
292-301: Slash amount parsed as U256 — good. Suggest echoing amount.- println!( - "Slashing collateral for executor {} with hotkey {}", - executor_id, hotkey - ); + println!( + "Slashing {} RAO for executor {} with hotkey {}", + slash_amount, executor_id, hotkey + );
490-493: Label nit: distinguish slash from other amounts.- println!(" Amount: {} RAO", slashed.slashAmount); + println!(" Slash amount: {} RAO", slashed.slashAmount);crates/basilica-validator/src/persistence/collateral_persistence.rs (1)
235-244: Tests updated to use slashAmount — good. Consider adding a miner persistence assertion.Add a test that sets a non-zero
slashed.minerand asserts it persisted.crates/collateral-contract/src/CollateralUpgradeable.sol (3)
239-241: Docs now drift from behavior — finalize no longer always clears executor mappingComments still say “removes the executor-to-miner mapping” and “fully closes the relationship”, but code only clears when remaining collateral is zero. Update the NatSpec to match the new semantics.
Apply this diff to adjust the docstring:
- /// @dev Transfers the collateral to the miner and removes the executor-to-miner mapping if successful - /// @dev This fully closes the relationship, allowing to request another reclaim + /// @dev Transfers the collateral to the miner. The executor-to-miner mapping is cleared only if remaining collateral becomes zero. + /// @dev This closes the pending reclaim; further reclaims can be initiated subject to available collateral.Also applies to: 278-280
263-276: Consider partial finalization instead of hard revert when slash reduced available collateralToday a trustee slash during a pending reclaim makes
finalizeReclaimpermanently revert until the trustee intervenes (deny). Consider paying outmin(reclaim.amount, currentCollateral)and closing the reclaim to avoid stuck states for end-users.Example adjustment:
- if (collaterals[hotkey][executorId] < amount) { - // miner got slashed and can't withdraw - revert InsufficientCollateralForReclaim(); - } - - collaterals[hotkey][executorId] -= amount; + uint256 payout = amount; + if (collaterals[hotkey][executorId] < payout) { + payout = collaterals[hotkey][executorId]; + } + if (payout == 0) revert InsufficientCollateralForReclaim(); + collaterals[hotkey][executorId] -= payout; ... - emit Reclaimed(reclaimRequestId, hotkey, executorId, miner, amount); + emit Reclaimed(reclaimRequestId, hotkey, executorId, miner, payout); ... - (bool success, ) = payable(miner).call{value: amount}(""); + (bool success, ) = payable(miner).call{value: payout}("");
366-375: Use custom errors instead of string requires for gas and consistencyElsewhere you use custom errors; align admin setters too for cheaper reverts and uniformity.
Example:
+ error ZeroAddressTrustee(); + error InvalidDecisionTimeout(); + error InvalidMinCollateralIncrease(); ... - require(newTrustee != address(0), "New trustee cannot be zero address"); + if (newTrustee == address(0)) revert ZeroAddressTrustee(); ... - require(newTimeout > 0, "Decision timeout must be greater than 0"); + if (newTimeout == 0) revert InvalidDecisionTimeout(); ... - require( - newMinIncrease > 0, - "Min collateral increase must be greater than 0" - ); + if (newMinIncrease == 0) revert InvalidMinCollateralIncrease();Also applies to: 381-389, 395-406
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
crates/basilica-validator/src/persistence/collateral_persistence.rs(4 hunks)crates/collateral-contract/README.md(2 hunks)crates/collateral-contract/flow.sh(1 hunks)crates/collateral-contract/src/CollateralUpgradableABI.json(3 hunks)crates/collateral-contract/src/CollateralUpgradeable.sol(5 hunks)crates/collateral-contract/src/lib.rs(3 hunks)crates/collateral-contract/src/main.rs(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
crates/collateral-contract/src/main.rs (1)
crates/collateral-contract/src/lib.rs (1)
deposit(184-203)
crates/basilica-validator/src/persistence/collateral_persistence.rs (1)
crates/collateral-contract/src/lib.rs (1)
from(38-54)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-validator (stable)
- GitHub Check: quick-checks
🔇 Additional comments (15)
crates/collateral-contract/flow.sh (1)
1-2: Shebang + strict mode — good hardening.crates/collateral-contract/src/CollateralUpgradableABI.json (3)
239-246: ABI: slashCollateral adds slashAmount — matches lib/CLI usage.
620-624: Event rename to slashAmount — consistent with consumers.
710-710: New error InsufficientCollateralForSlash is exposed — ensure CLI surfaces reverts.Confirm the CLI prints clear errors on this revert; today it prints a generic error. If desired, we can map this selector to a friendly message.
crates/collateral-contract/src/lib.rs (2)
22-23: Bytecode now 0x-prefixed — correct for alloy sol! attribute.
279-299: Approve: Public API now includes slash_amount; call sites updated.
Main usage updated at crates/collateral-contract/src/main.rs:305; no other references found.crates/collateral-contract/src/main.rs (5)
219-221: Better UX: RAO unit included — nice.
340-341: Min collateral printed in RAO — consistent.
374-376: Collateral printed in RAO — consistent.
385-386: Reclaim amount printed in RAO — consistent.
536-539: JSON field name: confirm downstream expects "amount" not "slashAmount".Staying with "amount" preserves older consumers, but it now semantically means "slashAmount". Verify no consumer expects "slashAmount" here.
crates/basilica-validator/src/persistence/collateral_persistence.rs (2)
8-8: Importing error for logging — fine.
359-360: Updated expectation after slashing — correct.crates/collateral-contract/src/CollateralUpgradeable.sol (2)
78-85: Event param rename has ABI surface implications — confirm indexers and clients are updatedRenaming
amount→slashAmountchanges the ABI JSON field name (though topics/signature stay the same). Ensure subgraphs, off-chain parsers, and tests that read by param name are updated.
104-105: Good guardrail: explicit error for over-slashIntroducing
InsufficientCollateralForSlashcleanly expresses the invariant and prevents underflow. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
crates/basilica-miner/tests/mod.rs (6)
220-226: Slash amount param added in correct position; strengthen assertions and exercise partial-slash behaviorGood alignment with the new API. Please also validate the new event field and resulting balance, and prefer a distinct variable for the slash amount to avoid ambiguity.
Apply this focused refactor inside this test to cover partial slashing and assert state/event invariants:
@@ - let executor_id: u128 = rand::thread_rng().gen_range(0..10000000000); - let hotkey: [u8; 32] = [1u8; 32]; - let amount = U256::from(10); + let executor_id: u128 = rand::thread_rng().gen_range(0..10000000000); + let hotkey: [u8; 32] = [1u8; 32]; + let deposit_amount = U256::from(10); + let slash_amount = U256::from(5); @@ - .value(amount); + .value(deposit_amount); @@ - // Start reclaim process + // Slash collateral (partial) @@ - let slash_tx = contract.slashCollateral( + let slash_tx = contract.slashCollateral( FixedBytes::from_slice(&hotkey), FixedBytes::from_slice(&executor_id.to_be_bytes()), - amount, + slash_amount, url.to_owned(), FixedBytes::from_slice(&url_checksum.to_be_bytes()), ); let slash_receipt = slash_tx.send().await?.get_receipt().await?; - slash_receipt.logs().iter().for_each(|log| { - if let Ok(event) = CollateralUpgradeable::Slashed::decode_log(&log.inner) { - assert_eq!(event.executorId, FixedBytes::from(executor_id)); - } - }); + let mut slashed_found = false; + slash_receipt.logs().iter().for_each(|log| { + if let Ok(event) = CollateralUpgradeable::Slashed::decode_log(&log.inner) { + assert_eq!(event.executorId, FixedBytes::from(executor_id)); + // New field per PR: ensure the emitted amount matches what we requested. + assert_eq!(event.slashAmount, slash_amount); + slashed_found = true; + } + }); + assert!(slashed_found); + + // Balance should decrease by exactly the slash amount. + let remaining = contract + .collaterals( + FixedBytes::from_slice(&hotkey), + FixedBytes::from_slice(&executor_id.to_be_bytes()), + ) + .call() + .await + .unwrap(); + assert_eq!(remaining, deposit_amount - slash_amount);
217-217: Misleading comment: this block performs a slash, not a reclaimRename to match the action to avoid confusion when scanning tests.
- // Start reclaim process + // Slash collateral (trustee-only onchain)
206-235: Add a negative/edge test for “slash > collateral” and zero-amount slashTo harden the new param contract, add cases that expect a revert for overslash (e.g., InsufficientCollateralForSlash) and for zero amount if disallowed by the contract.
I can add a sibling test (ignored) that deposits 10, attempts to slash 11 (expect revert), and attempts to slash 0 (expect revert/no-op per spec). Want me to push that?
20-35: Parameterize RPC URL/chain id via env for portabilityHardcoding Finney and chain id reduces reusability. Read from env with sensible defaults.
- let rpc_url = TESTNET_URL; + let rpc_url = std::env::var("BASILICA_RPC_URL").unwrap_or_else(|_| TESTNET_URL.to_string()); @@ - signer.set_chain_id(Some(945)); + let chain_id = std::env::var("BASILICA_CHAIN_ID").ok().and_then(|s| s.parse().ok()).unwrap_or(945u64); + signer.set_chain_id(Some(chain_id));
14-19: Docs nit: fix path typo and clarify notesMinor polish in the test docs.
- /// ~/.basilca/private_key is default for private key file path + /// ~/.basilica/private_key is the default private key file path
132-138: Comment mismatch: you’re checking Reclaimed, not DeniedAlign comment with the assertion.
- // Check for Denied event + // Check for Reclaimed event
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/basilica-miner/tests/mod.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-miner (stable)
- GitHub Check: build-validator (stable)
- GitHub Check: quick-checks
🔇 Additional comments (1)
crates/basilica-miner/tests/mod.rs (1)
220-226: All slashCollateral call sites use the 5-arg signature — resolved.
Found invocations in crates/collateral-contract/src/lib.rs and crates/basilica-miner/tests/mod.rs; both pass five arguments.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/collateral-contract/src/main.rs (3)
117-119: Fix typo in CLI help text."Amount to slash in in wei" → "Amount to slash in wei".
Apply this diff:
- /// Amount to slash in in wei + /// Amount to slash in wei
292-299: Validate non-zero slash amount and echo it for safety.Prevent accidental zero-value txs and make the CLI confirmation explicit.
Apply this diff:
let executor_uuid = Uuid::parse_str(&executor_id)?; - let amount_u256 = parse_u256(&slash_amount)?; + let amount_u256 = parse_u256(&slash_amount)?; + if amount_u256.is_zero() { + anyhow::bail!("slash_amount must be > 0 wei"); + } - println!( - "Slashing collateral for executor {} with hotkey {}", - executor_id, hotkey - ); + println!( + "Slashing {} wei of collateral for executor {} with hotkey {}", + slash_amount, executor_id, hotkey + ); collateral_contract::slash_collateral( &private_key, hotkey_bytes, executor_uuid.into_bytes(), - amount_u256, + amount_u256,Optional follow-up: make
slash_amountaU256field in the Clap struct so Clap parses/validates it natively, removing the manual parse.Also applies to: 309-309
536-536: ExposeslashAmountalongsideamountin JSON (preserve compatibility).Keep
amountfor existing consumers, but addslashAmountto reflect the event field rename.Apply this diff:
CollateralEvent::Slashed(slashed) => { serde_json::json!({ "type": "Slashed", "hotkey": hex::encode(slashed.hotkey.as_slice()), "executorId": hex::encode(slashed.executorId.as_slice()), "miner": slashed.miner.to_string(), - "amount": slashed.slashAmount.to_string(), + "amount": slashed.slashAmount.to_string(), + "slashAmount": slashed.slashAmount.to_string(), "url": slashed.url, "urlContentMd5Checksum": hex::encode(slashed.urlContentMd5Checksum.as_slice()) }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/collateral-contract/src/main.rs(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
crates/collateral-contract/src/main.rs (2)
crates/basilica-validator/src/persistence/collateral_persistence.rs (7)
deposit(107-107)deposit(108-108)deposit(127-127)deposit(128-128)slashed(171-171)slashed(172-172)slashed(194-194)crates/collateral-contract/src/lib.rs (1)
deposit(184-203)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-miner (stable)
- GitHub Check: build-validator (stable)
- GitHub Check: quick-checks
🔇 Additional comments (5)
crates/collateral-contract/src/main.rs (5)
219-219: LGTM: clearer unit in deposit message.
340-340: LGTM: unit clarified for min collateral increase.
374-374: LGTM: unit clarified for collateral query.
385-385: LGTM: unit clarified for reclaim details.
470-471: LGTM: consistent “in wei” across event pretty-prints.Also applies to: 481-482, 491-492
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
crates/collateral-contract/src/main.rs (4)
117-119: Fix typo in CLI help text.Duplicate word: "in in wei".
- /// Amount to slash in in wei + /// Amount to slash in wei
214-221: Also validate non-zero deposit amount for consistency.Mirror the slash check to prevent accidental zero-value deposits.
} => { let hotkey_bytes = parse_hotkey(&hotkey)?; let amount_u256 = parse_u256(&amount)?; let executor_uuid = Uuid::parse_str(&executor_id)?; + if amount_u256.is_zero() { + anyhow::bail!("amount must be > 0 wei"); + } + println!( "Depositing {} in wei for executor {} with hotkey {}", amount, executor_id, hotkey );
305-307: Make slash log consistent with “in wei” phrasing used elsewhere.Current message omits units.
- println!( - "Slashing collateral for executor {} with hotkey {} amount {}", - executor_id, hotkey, slash_amount - ); + println!( + "Slashing collateral for executor {} with hotkey {} amount {} in wei", + executor_id, hotkey, slash_amount + );
437-439: Hex input support for U256?
U256::from_strmay accept only decimal. If users pass0x..., this will fail. Either document “decimal only” or add hex support.Would you like me to patch
parse_u256to accept both decimal and 0x‑prefixed hex (with tests)?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/collateral-contract/src/main.rs(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
crates/collateral-contract/src/main.rs (2)
crates/collateral-contract/src/lib.rs (2)
slash_collateral(279-300)deposit(184-203)crates/basilica-validator/src/persistence/collateral_persistence.rs (7)
deposit(107-107)deposit(108-108)deposit(127-127)deposit(128-128)slashed(171-171)slashed(172-172)slashed(194-194)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-miner (stable)
- GitHub Check: build-validator (stable)
- GitHub Check: quick-checks
🔇 Additional comments (4)
crates/collateral-contract/src/main.rs (4)
288-296: Newslash_amountarg is correctly plumbed through Tx command.LGTM.
299-303: Good validation for non-zero slash amount.LGTM. Nice early bail-out.
219-221: Consistent “in wei” output across deposit/query/event paths.Nice UX polish; helps avoid unit confusion.
Also applies to: 343-344, 377-379, 389-389, 473-474, 485-485, 494-495
288-320: ```shell
#!/bin/bash
set -euo pipefailecho "PWD: $(pwd)"
echo "---- git status (first 50 lines) ----"
git status --porcelain | sed -n '1,50p' || trueecho
echo "---- ripgrep search (include hidden, ignore .gitignore) for JSON/JS/TS consumers ----"
if command -v rg >/dev/null 2>&1; then
rg --hidden --no-ignore -n -S -C2
-e '"type"\s*:\s*"Slashed"'
-e '\bslashAmount\b'
-e '"amount"\s*:\s*' || trueecho
echo "---- ripgrep search for Rust/other code references to Slashed / slash_amount / Slashed.amount ----"
rg --hidden --no-ignore -n -S -C2
-e '\bSlashed\b'
-e '\bslash_amount\b'
-e 'Slashed.amount' || trueecho
echo "---- ripgrep broader search for 'amount' occurrences ----"
rg --hidden --no-ignore -n -S -C2
-e '"amount"'
-e '\bamount\b' || true
else
echo "rg not found; falling back to find+grep (may be slower)"
find . -type f -not -path "./target/" -print0 | xargs -0 grep -n -E -C2 -e '"type"\s:\s*"Slashed"' -e '\bslashAmount\b' -e '"amount"\s*:\s*' || true
fiecho "Done."
</blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| "slashAmount": slashed.slashAmount.to_string(), | ||
| "url": slashed.url, | ||
| "urlContentMd5Checksum": hex::encode(slashed.urlContentMd5Checksum.as_slice()) | ||
| }) |
There was a problem hiding this comment.
Breaking JSON schema: renamed Slashed.amount → slashAmount. Provide backward‑compat alias or gate behind a flag.
Existing consumers expecting "amount" will break.
Option A (add alias now; deprecate later):
CollateralEvent::Slashed(slashed) => {
serde_json::json!({
"type": "Slashed",
"hotkey": hex::encode(slashed.hotkey.as_slice()),
"executorId": hex::encode(slashed.executorId.as_slice()),
"miner": slashed.miner.to_string(),
- "slashAmount": slashed.slashAmount.to_string(),
+ "slashAmount": slashed.slashAmount.to_string(),
+ "amount": slashed.slashAmount.to_string(), // backward-compat
"url": slashed.url,
"urlContentMd5Checksum": hex::encode(slashed.urlContentMd5Checksum.as_slice())
})
}Option B: Gate under a CLI flag (e.g., --json-version=2) and keep v1 shape by default.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "slashAmount": slashed.slashAmount.to_string(), | |
| "url": slashed.url, | |
| "urlContentMd5Checksum": hex::encode(slashed.urlContentMd5Checksum.as_slice()) | |
| }) | |
| "slashAmount": slashed.slashAmount.to_string(), | |
| "amount": slashed.slashAmount.to_string(), // backward-compat | |
| "url": slashed.url, | |
| "urlContentMd5Checksum": hex::encode(slashed.urlContentMd5Checksum.as_slice()) | |
| }) |
🤖 Prompt for AI Agents
crates/collateral-contract/src/main.rs around lines 539 to 542: the JSON output
renamed the field "amount" → "slashAmount", which breaks existing consumers;
either restore a backward-compatible alias or gate the new shape behind a
JSON-version flag. Fix option A (quick): when building the JSON object include
both keys, e.g. add "amount": slashed.slashAmount (marked deprecated in
comments) alongside "slashAmount" so v1 clients continue to work. Fix option B
(preferred for explicitness): add a CLI/config option (e.g., --json-version with
default "1"), keep current output for version 1, and when json-version=2 emit
"slashAmount" instead of "amount"; update help text and tests accordingly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/collateral-contract.md (2)
13-14: Tighten wording; fix grammar and casing; clarify key order.Proposed rewrite for clarity and consistency:
-The main adaptation is the basilica implementation extends the executorToMiner to double map, adding the hotkey as first dimension' key. And use the UUPSUpgradeable as upgrade framework, collateral contract can be upgraded in the future. +The main adaptations: Basilica extends `executorToMiner` to a double map keyed by (`hotkey`, `executorId`), and adopts OpenZeppelin’s `UUPSUpgradeable` pattern so the collateral contract can be upgraded without losing state.If the actual key order differs, adjust the tuple accordingly.
267-267: Clarify partial-slash behavior; fix grammar.-Miner will lose deposited some amount or all collateral for violated executor; miner need to deposit for that executor again if they want to keep getting rewards for executor. +The miner may lose some or all collateral associated with the violated executor. To continue earning rewards for that executor, the miner must redeposit to restore the required collateral.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/collateral-contract.md(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-miner (stable)
- GitHub Check: build-validator (stable)
- GitHub Check: quick-checks
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/collateral-contract.md (2)
205-221: Standardize on "Upgradeable" — fix the spelling mismatchFix inconsistent reference: crates/collateral-contract/src/lib.rs:25 references "./src/CollateralUpgradableABI.json" (missing 'e'); rename or update to "./src/CollateralUpgradeableABI.json" and ensure all filenames/types/docs use "Upgradeable".
113-116: Align docs with on-chain slashCollateral ABIDocs currently say "miner, slashAmount, executorUuid" — incorrect. Update to the contract signatures: CollateralUpgradeable (partial slash) -> (hotkey: bytes32, executorId: bytes16, slashAmount: uint256, url: string, urlContentMd5Checksum: bytes16); CollateralUpgradeableV2 / Collateral.sol (full slash) -> (hotkey: bytes32, executorId: bytes16, url: string, urlContentMd5Checksum: bytes16). Miner is derived internally, not an input. Confirm/replace the doc text and CLI calls to use hotkey/executorId ordering and the appropriate presence/absence of slashAmount.
Relevant locations: docs/collateral-contract.md (lines ~113–116), crates/collateral-contract/src/CollateralUpgradeable.sol (sig at ~325–331), crates/collateral-contract/src/CollateralUpgradeableV2.sol (sig at ~290–295), ABI JSON (crates/collateral-contract/src/CollateralUpgradableABI.json inputs ~236–241), and CLI call site crates/collateral-contract/src/lib.rs (~290–292).
🧹 Nitpick comments (5)
docs/collateral-contract.md (5)
13-14: Call out partial-slash mechanics and mapping-clearing rules hereMake the adaptation summary explicitly mention the new slashAmount parameter and executorToMiner clearing semantics.
-The main adaptations: Basilica extends `executorToMiner` to a double map keyed by (`hotkey`, `executorId`), and adopts OpenZeppelin’s `UUPSUpgradeable` pattern so the collateral contract can be upgraded without losing state. +Main adaptations: +- Extends `executorToMiner` to a double map keyed by (`hotkey`, `executorId`). +- Adopts OpenZeppelin’s `UUPSUpgradeable` pattern so the collateral contract can be upgraded without losing state. +- Introduces partial slashing: `slashCollateral(miner, executorId, slashAmount, ...)` validates and applies `slashAmount` (cannot exceed the executor’s current collateral). The (`hotkey`, `executorId`) entry is cleared only when the remaining collateral reaches zero.
62-64: MD5 is weak; document it as current and recommend SHA‑256 for integrityIf the implementation currently stores MD5, keep it documented but recommend a stronger hash to set expectations.
-Functions `slashCollateral`, `reclaimCollateral`, and `denyReclaim` include URL fields (and content MD5 checksums) to reference off-chain - explanations or evidence for each action, ensuring decisions are transparent and auditable. +Functions `slashCollateral`, `reclaimCollateral`, and `denyReclaim` include URL fields (and content hashes — currently MD5; prefer SHA‑256 in future upgrades) to reference off-chain + explanations or evidence for each action, ensuring decisions are transparent and auditable.
109-113: Fix duplicated/awkward sentences about confirming depositThere’s duplication and a grammar issue (“for that your executor”).
- - Confirm on-chain that your collateral has been successfully locked for that miner - - - Confirm on-chain that your collateral has been successfully locked for that your executor + - Confirm on-chain that your collateral has been successfully locked for your miner and the specified executor.
267-268: Tie consequences to partial‑slash behaviorExplicitly link the consequence to partial slashing and minimum-collateral thresholds.
-The miner may lose some or all collateral associated with the violated executor. To continue earning rewards for that executor, the miner must redeposit to restore the required collateral. +The miner may lose some or all collateral (via `slashAmount`) associated with the violated executor. If the remaining collateral falls below the required minimum for that executor, the miner must top up to resume earning rewards.
154-167: Clarify --amount is in base units (wei/integer)Confirmed: the CLI parses --amount into a U256 and README examples use base-unit integers (e.g. 1000000000000000000). Update docs/collateral-contract.md (lines 154–167) to state that --amount is in wei/base units and show a human-readable equivalent (e.g. 1 = 1000000000000000000, 1e18).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/collateral-contract.md(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint-complex
- GitHub Check: build-miner (stable)
- GitHub Check: build-validator (stable)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
docs/collateral-contract.md (3)
13-17: Fix stray "+" and align naming/order for API docs.
- Remove the leading "+" before "Main adaptations:".
- Keep parameter order consistent with the code/API:
slashCollateral(miner, executorId, slashAmount, ...). Elsewhere the doc usesslashAmountbeforeexecutorUuid(Line 115) — please standardize.- Unify identifier spelling across the doc: pick one of
executorId(preferred) orexecutorUuidand use it everywhere.Apply:
-+Main adaptations: +Main adaptations: - Extends `executorToMiner` to a double map keyed by (`hotkey`, `executorId`). - Adopts OpenZeppelin’s `UUPSUpgradeable` pattern so the collateral contract can be upgraded without losing state. -- Introduces partial slashing: `slashCollateral(miner, executorId, slashAmount, ...)` validates and applies `slashAmount` (cannot exceed the executor’s current collateral). The (`hotkey`, `executorId`) entry is cleared only when the remaining collateral reaches zero. +- Introduces partial slashing: `slashCollateral(miner, executorId, slashAmount, ...)` validates and applies `slashAmount` (cannot exceed the executor’s current collateral). The (`hotkey`, `executorId`) entry is cleared only when the remaining collateral reaches zero.
112-112: Grammar fix."for that your miner" → "for your miner".
- - Confirm on-chain that your collateral has been successfully locked for that your miner and the specified executor. + - Confirm on-chain that your collateral has been successfully locked for your miner and the specified executor.
268-268: Tighten wording and fix capitalization.Clarify partial‑slash behavior and fix sentence case.
-The miner may lose some or all collateral (via `slashAmount`) associated with the violated executor. the remaining collateral will continue to be used for miners. +The miner may lose some or all collateral (via `slashAmount`) associated with the violated executor. The remaining collateral continues to count toward eligibility and assignments until exhausted.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/collateral-contract.md(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-validator (stable)
- GitHub Check: build-miner (stable)
- GitHub Check: quick-checks
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/collateral-contract.md (2)
113-113: Use the same terms as the mapping and CLIReplace “miner”/“executor” with the exact identifiers used elsewhere (hotkey, executorId) to avoid ambiguity.
- - Confirm on-chain that your collateral has been successfully locked for your miner and the specified executor. + - Confirm on-chain that your collateral is locked for your hotkey and the specified executorId.
269-269: Clarify outcome of partial slash“The remaining collateral will continue to be used for miners” is unclear.
-The miner may lose some or all collateral (via `slashAmount`) associated with the violated executor. The remaining collateral will continue to be used for miners. +The miner loses `slashAmount` of collateral tied to the specified `executorId`. Any remaining collateral stays locked for that (`hotkey`, `executorId`) and continues to count toward eligibility/prioritization; the mapping entry is cleared only when the remaining collateral becomes zero.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/collateral-contract.md(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-validator (stable)
- GitHub Check: build-miner (stable)
- GitHub Check: quick-checks
🔇 Additional comments (1)
docs/collateral-contract.md (1)
13-18: Standardize executor identifier toexecutorId(UUIDv4); clarify slashAmount units and revert behaviorDocs currently mix
executorId/executor UUID/executorUuidand the CLI examples use--executor-id. UseexecutorIdas the canonical name, state it is a UUIDv4, document thatslashAmountis denominated in the token’s smallest unit, and state thatslashCollateral(...)reverts (e.g.,InsufficientCollateralForSlash) if slashAmount > current collateral.Occurrences to update: docs/collateral-contract.md — lines ~15-18, 47-48, 111-117, 118-120, 167-169, 185-186, 191-193.
Apply within this section:
- - Extends `executorToMiner` to a double map keyed by (`hotkey`, `executorId`). + - Extends `executorToMiner` to a double map keyed by (`hotkey`, `executorId`), where `executorId` is a UUIDv4. - - Introduces partial slashing: `slashCollateral(miner, executorId, slashAmount, ...)` validates and applies `slashAmount` (cannot exceed the executor’s current collateral). The (`hotkey`, `executorId`) entry is cleared only when the remaining collateral reaches zero. + - Introduces partial slashing: `slashCollateral(miner, executorId, slashAmount, ...)` validates and applies `slashAmount` (denominated in the token’s smallest unit) and reverts (e.g., `InsufficientCollateralForSlash`) if it exceeds the executor’s current collateral. The (`hotkey`, `executorId`) entry is cleared only when remaining collateral reaches zero.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
docs/collateral-contract.md (4)
13-18: Clarify partial slashing invariants and align terminology.Good high-level bullets. Please make the invariants and renamed fields explicit, and keep “executorId” naming consistent.
Apply:
- - Introduces partial slashing: `slashCollateral(miner, executorId, slashAmount, ...)` validates and applies `slashAmount` (cannot exceed the executor’s current collateral). The (`hotkey`, `executorId`) entry is cleared only when the remaining collateral reaches zero. + - Introduces partial slashing: `slashCollateral(miner, executorId, slashAmount, ...)` validates and applies `slashAmount` (must be ≤ current collateral). Remaining collateral is `remaining = previous - slashAmount`; the (`hotkey`, `executorId`) entry is cleared only when `remaining == 0`. + - Renames event field `Slashed.amount` → `Slashed.slashAmount`; introduces `InsufficientCollateralForSlash` to prevent over‑slashing.Optionally add a short note that upgrades use UUPS and require a gated
onlyProxy+ upgrade authorization (e.g.,onlyAdmin/onlyTrustee) to avoid accidental bricking.
113-113: Term consistency: use “executorId” everywhere (drop “UUID/executorUuid”).This line is consistent, but other sections still say “executor UUID/executorUuid” (e.g., Lines 47, 112, 116, 119). Please standardize to “executorId” to match the CLI flags (
--executor-id) and mapping keys.If you prefer “executorUuid” as canonical, flip the change here instead and update CLI/docs accordingly. I can generate a search-and-replace patch if helpful.
116-116: FixslashCollateralarg order and naming to match ABI/CLI.Docs say “miner, slashAmount, executorUuid…”. Elsewhere you use
(miner, executorId, slashAmount, ...). Keep one order and one name.Apply:
- If a miner is found violating subnet rules (e.g., returning invalid responses), the subnet owner (admin) or an authorized slasher can penalize a misbehaving miner by slashing some or all of the miner's collateral. - If a miner is found violating subnet rules (e.g., returning invalid responses), the subnet owner (admin) or an authorized slasher **calls** `slashCollateral()` with the `miner`, `slashAmount`, `executorUuid`, and justification details to reduce the miner’s collateral. + If a miner is found violating subnet rules (e.g., returning invalid responses), the subnet owner (admin) or an authorized slasher **calls** `slashCollateral(miner, executorId, slashAmount, url, contentMd5)` to reduce the miner’s collateral.Also add a CLI example using
--executor-idand--slash-amountfor parity.
269-269: Nice: precise description of partial slashing behavior. Add guard/error mention.This is accurate. Consider appending a brief note that over‑slashing reverts with
InsufficientCollateralForSlashso users know why a slash might fail.Example addition:
- The miner loses `slashAmount` of collateral tied to the specified `executorId`. Any remaining collateral stays locked for that (`hotkey`, `executorId`) and continues to count toward eligibility/prioritization; the mapping entry is cleared only when the remaining collateral becomes zero. + The miner loses `slashAmount` of collateral tied to the specified `executorId`. Any remaining collateral stays locked for that (`hotkey`, `executorId`) and continues to count toward eligibility/prioritization; the mapping entry is cleared only when the remaining collateral becomes zero. Attempts to slash more than the current collateral revert with `InsufficientCollateralForSlash`.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/collateral-contract.md(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-miner (stable)
- GitHub Check: build-validator (stable)
- GitHub Check: quick-checks
|
close it, the code change will be included in #164 |
1 participant
Summary
Related Issues
Closes #(issue number)
Type of Change
Changes Made
List the main changes in this PR:
Testing
How Has This Been Tested?
Describe the tests you ran to verify your changes.
cargo test)Test Configuration
Checklist
cargo fmtto format my codecargo clippyand addressed all warningsAdditional Context
Add any other context about the PR here.
Summary by CodeRabbit
New Features
CLI
Bug Fixes
Documentation
Tests