Skip to content

feat: enforce ordering of allowance check #6602

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 22, 2025
Merged

feat: enforce ordering of allowance check #6602

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f0fb390
feat: enforce ordering of allowance check
obycode Oct 14, 2025
b22dcac
chore: clippy suggestion
obycode Oct 14, 2025
4fb73a0
refactor: avoid sort
obycode Oct 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 40 additions & 42 deletions clarity/src/vm/functions/post_conditions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,13 @@
allowances: Vec<Allowance>,
assets: &AssetMap,
) -> InterpreterResult<Option<u128>> {
let mut earliest_violation: Option<u128> = None;
let mut record_violation = |candidate: u128| {
if earliest_violation.map_or(true, |current| candidate < current) {

Check failure on line 389 in clarity/src/vm/functions/post_conditions.rs

View workflow job for this annotation

GitHub Actions / Clippy Check 

this `map_or` can be simplified
earliest_violation = Some(candidate);
}
};

// Elements are (index in allowances, amount)
let mut stx_allowances: Vec<(usize, u128)> = Vec::new();
// Map assets to a vector of (index in allowances, amount)
Expand Down Expand Up @@ -428,34 +435,30 @@

// Check STX movements
if let Some(stx_moved) = assets.get_stx(owner) {
// If there are no allowances for STX, any movement is a violation
if stx_allowances.is_empty() {
return Ok(Some(MAX_ALLOWANCES as u128));
}

// Check against the STX allowances
for (index, allowance) in &stx_allowances {
if stx_moved > *allowance {
return Ok(Some(u128::try_from(*index).map_err(|_| {
InterpreterError::Expect("failed to convert index to u128".into())
})?));
// If there are no allowances for STX, any movement is a violation
record_violation(MAX_ALLOWANCES as u128);
} else {
for (index, allowance) in &stx_allowances {
if stx_moved > *allowance {
record_violation(*index as u128);
break;
}
}
}
}

// Check STX burns
if let Some(stx_burned) = assets.get_stx_burned(owner) {
// If there are no allowances for STX, any burn is a violation
if stx_allowances.is_empty() {
return Ok(Some(MAX_ALLOWANCES as u128));
}

// Check against the STX allowances
for (index, allowance) in &stx_allowances {
if stx_burned > *allowance {
return Ok(Some(u128::try_from(*index).map_err(|_| {
InterpreterError::Expect("failed to convert index to u128".into())
})?));
// If there are no allowances for STX, any burn is a violation
record_violation(MAX_ALLOWANCES as u128);
} else {
for (index, allowance) in &stx_allowances {
if stx_burned > *allowance {
record_violation(*index as u128);
break;
}
}
}
}
Expand All @@ -479,17 +482,17 @@

if merged.is_empty() {
// No allowance for this asset, any movement is a violation
return Ok(Some(MAX_ALLOWANCES as u128));
record_violation(MAX_ALLOWANCES as u128);
continue;
}

// Sort by allowance index so we check allowances in order
merged.sort_by_key(|(idx, _)| *idx);

for (index, allowance) in merged {
if *amount_moved > allowance {
return Ok(Some(u128::try_from(index).map_err(|_| {
InterpreterError::Expect("failed to convert index to u128".into())
})?));
record_violation(index as u128);
break;
}
}
}
Expand All @@ -512,20 +515,17 @@

if merged.is_empty() {
// No allowance for this asset, any movement is a violation
return Ok(Some(MAX_ALLOWANCES as u128));
record_violation(MAX_ALLOWANCES as u128);
continue;
}

// Sort by allowance index so we check allowances in order
merged.sort_by_key(|(idx, _)| *idx);

for (index, allowance_vec) in merged {
// Check against the NFT allowances
for id_moved in ids_moved {
if !allowance_vec.contains(id_moved) {
return Ok(Some(u128::try_from(index).map_err(|_| {
InterpreterError::Expect("failed to convert index to u128".into())
})?));
}
if ids_moved.iter().any(|id| !allowance_vec.contains(id)) {
record_violation(index as u128);
break;
}
}
}
Expand All @@ -535,20 +535,18 @@
if let Some(stx_stacked) = assets.get_stacking(owner) {
// If there are no allowances for stacking, any stacking is a violation
if stacking_allowances.is_empty() {
return Ok(Some(MAX_ALLOWANCES as u128));
}

// Check against the stacking allowances
for (index, allowance) in &stacking_allowances {
if stx_stacked > *allowance {
return Ok(Some(u128::try_from(*index).map_err(|_| {
InterpreterError::Expect("failed to convert index to u128".into())
})?));
record_violation(MAX_ALLOWANCES as u128);
} else {
for (index, allowance) in &stacking_allowances {
if stx_stacked > *allowance {
record_violation(*index as u128);
break;
}
}
}
}

Ok(None)
Ok(earliest_violation)
}

/// Handles all allowance functions, always returning an error, since these are
Expand Down
36 changes: 36 additions & 0 deletions clarity/src/vm/tests/post_conditions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1404,3 +1404,39 @@ fn test_restrict_assets_with_error_in_body() {
ClarityError::ShortReturn(ShortReturnType::ExpectedValue(expected_err.into()));
assert_eq!(short_return, execute(snippet).unwrap_err());
}

#[test]
fn test_restrict_assets_with_multiple_violations_different_kinds() {
let snippet = r#"
(define-non-fungible-token stackaroo uint)
(nft-mint? stackaroo u122 tx-sender)
(nft-mint? stackaroo u123 tx-sender)
(let ((recipient 'SP000000000000000000002Q6VF78))
(restrict-assets? tx-sender ((with-nft current-contract "stackaroo" (list u122)) (with-stx u10))
(begin
(try! (stx-transfer? u50 tx-sender recipient))
(try! (nft-transfer? stackaroo u123 tx-sender recipient))
)
)
)"#;
let expected = Value::error(Value::UInt(0)).unwrap();
assert_eq!(expected, execute(snippet).unwrap().unwrap());
}

#[test]
fn test_restrict_assets_with_multiple_violations_different_kinds_order_2() {
let snippet = r#"
(define-non-fungible-token stackaroo uint)
(nft-mint? stackaroo u122 tx-sender)
(nft-mint? stackaroo u123 tx-sender)
(let ((recipient 'SP000000000000000000002Q6VF78))
(restrict-assets? tx-sender ((with-stx u10) (with-nft current-contract "stackaroo" (list u122)))
(begin
(try! (nft-transfer? stackaroo u123 tx-sender recipient))
(try! (stx-transfer? u50 tx-sender recipient))
)
)
)"#;
let expected = Value::error(Value::UInt(0)).unwrap();
assert_eq!(expected, execute(snippet).unwrap().unwrap());
}
Loading