[Clarity-4] Implement contract post-conditions #6516
Conversation
obycode
force-pushed
the
feat/restrict-assets
branch
2 times, most recently
from
9a75478 to
e02ca38
Compare
|
In implementing this, I'm wondering if it might be better to make a small change to the (with-nft contract-id:principal token-name:(string-ascii 128 identifier:T)I'm wondering if it makes more sense to change that to allow a list of identifiers: (with-nft contract-id:principal token-name:(string-ascii 128 identifier:(list T 128))This would allow the list to be built within the code and then passed to a single allowance expression for each NFT. |
obycode
force-pushed
the
feat/restrict-assets
branch
4 times, most recently
from
efeeab5 to
b922ae9
Compare
This just adds the basic support for the syntax and some setup for the full implementation.
obycode
force-pushed
the
feat/restrict-assets
branch
from
663c1e9 to
b450cd1
Compare
That infrastructure is not setup for handling PoX state, and the stacking tracking in the asset maps is not setup yet any way.
|
Remaining work is to handle the |
|
Remaining tasks now are:
|
clarity/src/vm/analysis/type_checker/v2_1/natives/post_conditions.rs
Outdated
Show resolved
Hide resolved
Thanks @hstove! I actually forgot about those TODOs I had at the bottom. The remaining tests I planned to add were:
Do you see any others that should get added? |
We decided to remove this because it doesn't really make sense.
|
@hstove TODOs resolved, additional tests added, and merge conflicts resolved. |
| if allowance_list.len() > MAX_ALLOWANCES { | ||
| return Err(CheckErrors::TooManyAllowances(MAX_ALLOWANCES, allowance_list.len()).into()); | ||
| } |
There was a problem hiding this comment.
Is allowance_list.len() == 0 permitted?
There was a problem hiding this comment.
Yes, it should be. That means this block of code can't move any assets.
| if allowance_list.len() > MAX_ALLOWANCES { | ||
| return Err(CheckErrors::TooManyAllowances(MAX_ALLOWANCES, allowance_list.len()).into()); | ||
| } |
There was a problem hiding this comment.
Same as above -- allowance_list.len == 0?
There was a problem hiding this comment.
Yes, 0 should be allowed. That means this block of code can't move any assets.
| let TypeSignature::SequenceType(SequenceSubtype::ListType(list_data)) = id_list_ty else { | ||
| return Err(CheckErrors::WithNftExpectedListOfIdentifiers.into()); | ||
| }; |
There was a problem hiding this comment.
Just to confirm -- NFT asset identifier types are not checked against the contract's definition (i.e., if my NFT uses uints, but I supply int identifiers in the restrict-assets?, the type checker won't complain).
This makes sense to me (checking this seems hard), but it'd be good to include this in documentation/spec and tests.
There was a problem hiding this comment.
Yeah, I left it out because I didn't think it was worth the overhead and complexity. I can definitely add that to the docs and spec.
| if function_name == "stack-increase" { | ||
| global_context.log_stacking(&stacker, new_balance.amount_locked())?; | ||
| } |
There was a problem hiding this comment.
Is this logging the new total amount? Or is it the increased amount?
| match name.as_str() { | ||
| "with-stx" => { |
There was a problem hiding this comment.
In the type-checker you use the NativeFunctions enum to do this match -- should we do that here too?
| allowance_list.len(), | ||
| )?; | ||
|
|
||
| let mut allowances = Vec::with_capacity(allowance_list.len()); |
There was a problem hiding this comment.
Probably wise to check the allowance list length fits in max allowable before this (even though we check in the type checker).
| let asset_maps = env.global_context.get_readonly_asset_map()?; | ||
|
|
||
| // If the allowances are violated: | ||
| // - Rollback the context | ||
| // - Emit an event | ||
| if let Some(violation_index) = check_allowances(&asset_owner, &allowances, asset_maps)? { |
There was a problem hiding this comment.
I think we want to guard these ? expressions as well -- or ensure that they're Expects errors (which I think they are). Because we wouldn't want a runtime-includable error here to return before the roll_back invocation.
| for allowance in allowance_list { | ||
| allowances.push(eval_allowance(allowance, env, context)?); | ||
| } |
There was a problem hiding this comment.
I think we should track the memory use of allowances (they're essentially variable bindings), and drop their memory consumption when we leave the restrict-assets/as-contract expression.
| match check_allowances(&contract_principal, &allowances, asset_maps) { | ||
| Ok(None) => {} | ||
| Ok(Some(violation_index)) => { | ||
| nested_env.global_context.roll_back()?; | ||
| return Value::error(Value::UInt(violation_index)); | ||
| } | ||
| Err(e) => { | ||
| nested_env.global_context.roll_back()?; | ||
| return Err(e); | ||
| } | ||
| } |
There was a problem hiding this comment.
This is semantically different than the handler in restrict-assets (in restrict-assets, the error case of check_allowances is handled with ?).
I don't think the semantic difference matters (because I think in all cases, we're talking about Expects errors), but I think we should figure out which approach we want to take, and just use one.
| for id in &nft.asset_ids { | ||
| set.insert(id.serialize_to_hex()?); | ||
| } |
There was a problem hiding this comment.
Can you use ClarityValue directly instead of serializing? Or you do want to have something hashable?
Implements contract post-conditions as described in #6396 and SIP PR stacksgov/sips#218.