fix: reject negative TAO amounts in staking and transfer params#9
fix: reject negative TAO amounts in staking and transfer params#9sunnysmol wants to merge 2 commits intoone-covenant:mainfrom
Conversation
WalkthroughAdds an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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.
🧹 Nitpick comments (1)
bittensor-rs/src/extrinsics/staking.rs (1)
44-55: Core validation logic is correct; consider handling NaN and Infinity edge cases.The negative amount check correctly prevents the integer overflow issue described in Issue
#8. However, special floating-point values likeNaNandf64::INFINITYwould pass this check and could produce unexpected results:
f64::NAN * 1e9 as u64→0(saturating cast)f64::INFINITY * 1e9 as u64→u64::MAX🔧 Optional: Add validation for special float values
pub fn new_tao(hotkey: &str, netuid: u16, amount_tao: f64) -> Result<Self, BittensorError> { - if amount_tao < 0.0 { + if amount_tao < 0.0 || !amount_tao.is_finite() { return Err(BittensorError::InvalidAmount { - reason: format!("TAO amount must be non-negative, got {}", amount_tao), + reason: format!("TAO amount must be a non-negative finite number, got {}", amount_tao), }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bittensor-rs/src/extrinsics/staking.rs` around lines 44 - 55, The constructor new_tao currently only checks for negative values but allows NaN/Infinity and can overflow when converting to amount_rao; update validation to reject non-finite floats (use amount_tao.is_finite()) and ensure the scaled value (amount_tao * 1_000_000_000.0) fits in u64 (compare against u64::MAX as f64) before casting, returning BittensorError::InvalidAmount if the checks fail so amount_rao remains safe.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@bittensor-rs/src/extrinsics/staking.rs`:
- Around line 44-55: The constructor new_tao currently only checks for negative
values but allows NaN/Infinity and can overflow when converting to amount_rao;
update validation to reject non-finite floats (use amount_tao.is_finite()) and
ensure the scaled value (amount_tao * 1_000_000_000.0) fits in u64 (compare
against u64::MAX as f64) before casting, returning BittensorError::InvalidAmount
if the checks fail so amount_rao remains safe.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2e0abeb1-38ea-4579-8ecf-0b9c708f572d
📒 Files selected for processing (3)
bittensor-rs/src/error.rsbittensor-rs/src/extrinsics/staking.rsbittensor-rs/src/extrinsics/transfer.rs
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
bittensor-rs/src/extrinsics/staking.rs (1)
236-258: Assert the new error variant explicitly in these regressions.
is_err()is too loose here; these tests should pinErr(BittensorError::InvalidAmount { .. })so they fail ifnew_tao()starts returning a different error.Suggested assertion pattern
- assert!(result.is_err()); + assert!(matches!(result, Err(BittensorError::InvalidAmount { .. })));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bittensor-rs/src/extrinsics/staking.rs` around lines 236 - 258, Update the tests that currently call StakeParams::new_tao to assert the specific error variant instead of using is_err(): replace assert!(result.is_err()) with an explicit pattern matching or assert_eq! that checks for Err(BittensorError::InvalidAmount { .. }) so the three tests (test_stake_params_negative_tao, test_stake_params_nan_tao, test_stake_params_infinity_tao) fail if a different error is returned; ensure you reference the BittensorError::InvalidAmount variant and StakeParams::new_tao in the assertion logic.bittensor-rs/src/extrinsics/transfer.rs (1)
232-253: Make the invalid-input tests matchInvalidAmount.These assertions currently pass for any error. Matching
Err(BittensorError::InvalidAmount { .. })would keep the tests anchored to the behavior this change is introducing.Suggested assertion pattern
- assert!(result.is_err()); + assert!(matches!(result, Err(BittensorError::InvalidAmount { .. })));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bittensor-rs/src/extrinsics/transfer.rs` around lines 232 - 253, The tests for invalid TAO inputs call TransferParams::new_tao but only assert that an Err is returned; update each test (test_transfer_params_negative_tao, test_transfer_params_nan_tao, test_transfer_params_infinity_tao) to match the specific error variant Err(BittensorError::InvalidAmount { .. }) instead of any Err, so they assert the intended InvalidAmount behavior introduced by the change; locate the tests and replace assert!(result.is_err()) with a pattern match or assert_eq! against Err(BittensorError::InvalidAmount { .. }) for each case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bittensor-rs/src/extrinsics/staking.rs`:
- Around line 44-54: In new_tao, add an upper-bound validation before casting
amount_tao to amount_rao to prevent overflow: compute the maximum representable
TAO as (u64::MAX as f64) / 1_000_000_000.0 and if amount_tao > max_tao return
Err(BittensorError::InvalidAmount { reason: format!(...) }); then only perform
the multiplication and as u64 cast when amount_tao is within [0.0, max_tao];
update the error message to mention the exceedance and the max allowed value and
add a regression test that calls new_tao with an oversized positive input (e.g.
> max_tao) asserting it returns the InvalidAmount error.
In `@bittensor-rs/src/extrinsics/transfer.rs`:
- Around line 42-52: new_tao currently only validates the TAO f64 input but then
scales and casts to u64 which can overflow silently; update new_tao to compute
the scaled value (amount_tao * 1_000_000_000.0) and verify it is finite and <=
u64::MAX (and >= 0) before casting, returning BittensorError::InvalidAmount if
out of range, and assign amount_rao only after that safe check; also add a
regression test that calls new_tao with a large finite TAO that would exceed u64
when scaled and asserts it returns InvalidAmount.
---
Nitpick comments:
In `@bittensor-rs/src/extrinsics/staking.rs`:
- Around line 236-258: Update the tests that currently call StakeParams::new_tao
to assert the specific error variant instead of using is_err(): replace
assert!(result.is_err()) with an explicit pattern matching or assert_eq! that
checks for Err(BittensorError::InvalidAmount { .. }) so the three tests
(test_stake_params_negative_tao, test_stake_params_nan_tao,
test_stake_params_infinity_tao) fail if a different error is returned; ensure
you reference the BittensorError::InvalidAmount variant and StakeParams::new_tao
in the assertion logic.
In `@bittensor-rs/src/extrinsics/transfer.rs`:
- Around line 232-253: The tests for invalid TAO inputs call
TransferParams::new_tao but only assert that an Err is returned; update each
test (test_transfer_params_negative_tao, test_transfer_params_nan_tao,
test_transfer_params_infinity_tao) to match the specific error variant
Err(BittensorError::InvalidAmount { .. }) instead of any Err, so they assert the
intended InvalidAmount behavior introduced by the change; locate the tests and
replace assert!(result.is_err()) with a pattern match or assert_eq! against
Err(BittensorError::InvalidAmount { .. }) for each case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 90fd56be-f14b-4ff4-ba62-6ed4d3ed4c76
📒 Files selected for processing (2)
bittensor-rs/src/extrinsics/staking.rsbittensor-rs/src/extrinsics/transfer.rs
| pub fn new_tao(hotkey: &str, netuid: u16, amount_tao: f64) -> Result<Self, BittensorError> { | ||
| if !amount_tao.is_finite() || amount_tao < 0.0 { | ||
| return Err(BittensorError::InvalidAmount { | ||
| reason: format!("TAO amount must be a non-negative finite number, got {}", amount_tao), | ||
| }); | ||
| } | ||
| Ok(Self { | ||
| hotkey: hotkey.to_string(), | ||
| netuid, | ||
| amount_rao: (amount_tao * 1_000_000_000.0) as u64, | ||
| } | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the staking.rs file to verify the implementation
find . -name "staking.rs" -type f | head -5Repository: one-covenant/bittensor-rs
Length of output: 109
🏁 Script executed:
# Read the constructor implementation and surrounding code
cat -n bittensor-rs/src/extrinsics/staking.rs | sed -n '40,60p'Repository: one-covenant/bittensor-rs
Length of output: 1004
🏁 Script executed:
# Check for any bounds validation elsewhere in the file
rg "18\.446|u64::MAX|overflow" bittensor-rs/src/extrinsics/staking.rsRepository: one-covenant/bittensor-rs
Length of output: 51
🏁 Script executed:
# Look for test cases related to StakeParams
rg -A 10 "fn.*test.*new_tao|#\[test\]" bittensor-rs/src/extrinsics/staking.rs | head -100Repository: one-covenant/bittensor-rs
Length of output: 1243
🏁 Script executed:
# Search for any validation of upper bounds (e.g., max TAO value)
rg "amount_tao.*<|>.*amount_tao|max.*tao" bittensor-rs/src/extrinsics/staking.rs -iRepository: one-covenant/bittensor-rs
Length of output: 222
Add upper-bound validation before casting amount_tao to prevent silent overflow.
The current validation checks is_finite() and >= 0.0, but this does not prevent values from overflowing when scaled by 1,000,000,000. For example, any amount_tao > ~18.4 billion will exceed u64::MAX after scaling, causing silent truncation or wraparound in the as u64 cast. Add a check to reject values above the representable ceiling (approximately 18.446 TAO) and add a regression test for oversized positive inputs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bittensor-rs/src/extrinsics/staking.rs` around lines 44 - 54, In new_tao, add
an upper-bound validation before casting amount_tao to amount_rao to prevent
overflow: compute the maximum representable TAO as (u64::MAX as f64) /
1_000_000_000.0 and if amount_tao > max_tao return
Err(BittensorError::InvalidAmount { reason: format!(...) }); then only perform
the multiplication and as u64 cast when amount_tao is within [0.0, max_tao];
update the error message to mention the exceedance and the max allowed value and
add a regression test that calls new_tao with an oversized positive input (e.g.
> max_tao) asserting it returns the InvalidAmount error.
| pub fn new_tao(dest: &str, amount_tao: f64) -> Result<Self, BittensorError> { | ||
| if !amount_tao.is_finite() || amount_tao < 0.0 { | ||
| return Err(BittensorError::InvalidAmount { | ||
| reason: format!("TAO amount must be a non-negative finite number, got {}", amount_tao), | ||
| }); | ||
| } | ||
| Ok(Self { | ||
| dest: dest.to_string(), | ||
| amount_rao: (amount_tao * 1_000_000_000.0) as u64, | ||
| keep_alive: true, | ||
| } | ||
| }) |
There was a problem hiding this comment.
Validate the scaled RAO range as well, not just the raw TAO input.
The current guard still accepts large finite TAO amounts whose scaled RAO value no longer fits in u64. That means new_tao() can still silently build an invalid amount_rao via the as u64 cast instead of failing fast with InvalidAmount. Please add an explicit upper-bound check and a regression test for an oversized finite amount.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bittensor-rs/src/extrinsics/transfer.rs` around lines 42 - 52, new_tao
currently only validates the TAO f64 input but then scales and casts to u64
which can overflow silently; update new_tao to compute the scaled value
(amount_tao * 1_000_000_000.0) and verify it is finite and <= u64::MAX (and >=
0) before casting, returning BittensorError::InvalidAmount if out of range, and
assign amount_rao only after that safe check; also add a regression test that
calls new_tao with a large finite TAO that would exceed u64 when scaled and
asserts it returns InvalidAmount.
1 participant
Fixes #8
Rejects negative TAO values early in
new_tao()to prevent integer overflow in downstream staking and transfer operations.Changes:
error.rs: AddedInvalidAmount { reason: String }error variantstaking.rs:new_tao()now returnsResult<Self, BittensorError>and rejects negative inputstransfer.rs: Same validation patternThis is a minimal, focused fix that addresses the root cause without changing any business logic.
Summary by CodeRabbit
New Features
Bug Fixes
Breaking Changes