Feature/resource movements static analysis #1909
Conversation
…to feature/resource-movements-static-analysis
|
Docker tags |
Benchmark for fb8c729Click to view benchmark
|
0xOmarA
changed the base branch from
feature/subintent-validation
to
develop
There was a problem hiding this comment.
Really nice work! Have left a few comments. Haven't looked through it with a fine tooth-comb.
I suggest when you're back you can rebase on develop and we can have a chat and possibly do some markups I can review it properly when I get your thumbs up that it's good to go :)
| /// A [`ManifestInterpretationVisitor`] that statically tracks the resources in the worktop and | ||
| /// reports the account withdraws and deposits made. | ||
| pub struct StaticResourceMovementsVisitor { | ||
| // TODO: How can we do this better? How can we abstract this so that it just does the static |
There was a problem hiding this comment.
I think it would help to start by thinking about:
- What are sensible general-purpose outputs for this? (which e.g. might also benefit other programmatic manifest analyzers, e.g. intent aggregators)
- What does the wallet want to know?
In terms of general needs:
- To power high-level aggregated review:
- For each line, it could be that we want to know: "Who is the other counterparty? (e.g. Intent or Blueprint or Object)? What are the constraints on inbound / outbound resources to/from that counterparty"?
- This could power account deposits/withdrawal constraints for the purpose of wallet review
- We probably also want account proofs created?
- For each line, it could be that we want to know: "Who is the other counterparty? (e.g. Intent or Blueprint or Object)? What are the constraints on inbound / outbound resources to/from that counterparty"?
- To sensibly replace / sensible combine with the manifest preview.
- In fact - it would be nice if optionally previewed worktop changes can be fed into this, and possibly used to have a third "previewed" field sitting somewhere in the middle of the bounds (hopefully! unless our logic is wrong :P), which can be added to the output to help guide the guarantees. Then you could just use this in the toolkit for either subintents or transaction intents - they'd always be bounds, and optionally a suggested preview amount for guarantee purposes?
- To power a static / detailed manifest review:
- For each line, what possibilities are there? And what impact do they have?
- e.g. Assertion fails / Assertion succeeds. Are either of these impossible?
- e.g. Deposit succeeds / Deposit refunds.
- What is the history/timeline per bucket (of constraints, and reasons for changes)?
- What is the history/timeline per worktop resource (of constraints, and reasons for changes)?
- For each line, what possibilities are there? And what impact do they have?
There was a problem hiding this comment.
There's also more advanced possibilities / needs.
To help an intent aggregator combine intents, they really want to know:
- What constraints are there on resource inputs to the manifest for the manifest to succeed? (from any invocation source).
But I think the two outputs above are roughly enough to do this! :).
As a first pass, manifests with ASSERT_WORKTOP_CONTAINS_EXACTLY before/after a YIELD_TO_PARENT might be all they handle.
The below is going to be off-topic, so you can ignore it. I couldn't help myself but explore this problem a bit.
In a general case, it's a little bit mind-bendy to think about... With the right model, it's not impossible:
- In one view, it's just a constraint system which you can probably through a general purpose algorithm at.
- In another view, it's just requirement chasing and consolidation - exploring the possibility tree, pruning very aggressively, and maintaining a set of constraints against inbound resources as we go. There's basically 0 unprunable branches in the tree, which should make it quite performant, if you only known what to care about...
But also it's far too complicated for right now and there's absolutely zero expectation that we'll do this for now. I just wanted to think it through briefly to check that in principle we're storing the kind of information that such a constraint solver / requirement chasing machine would want.
But anyway, I explored this, so I'll dump some thoughts down. As a general purpose example:
0. YIELD_TO_PARENT;
1. TAKE_FROM_WORKTOP [XRD] 1 Bucket("xrd1");
2. CALL_METHOD "try_deposit_or_abort" ... Bucket("xrd1");
3. YIELD_TO_PARENT;
4. TAKE_FROM_WORKTOP [XRD] 5 Bucket("xrd2");
5. CALL_METHOD "deposit" ... Bucket("xrd2");
6. TAKE_ALL_FROM_WORKTOP [JOBU] Bucket("jobu");
7. CALL_METHOD "deposit_or_refund" Bucket("jobu");
8. ASSERT_WORKTOP_CONTAINS [JOBU] 2;
9. YIELD_TO_PARENT Expression("ENTIRE_WORKTOP");
This would output the following requirements for received resources by line:
- Line 1 requires 1 XRD on the worktop, which chasing back requires:
- XRD: Inbound[L0] - 1 >= 0 (AKA. the parent send at least 1 XRD to the manifest on line 0)
- Line 4 requires 5 XRD on the worktop, which chasing back requires:
- XRD: Inbound[L0] + Inbound[L3] - 6 >= 0
- Line 8: Requires 2 JOBU on the worktop, which chasing back requires JOBU: Inbound[L7] == 2... but actually this is a native call, so we can replace this requirement with two cases (Case 1) No refund (impossible as then Inbound[L7] = 0, so prune this) and (Case 2) Refund occurs (possible) && Bucket["jobu"] == 2... which resolves to
- JOBU: Inbound[L0] + Inbound[L3] -2 >= 0
And can expect the following resources to be exchanged, in the only unpruned path in the possibility space:
- Line 0: Parent:
- XRD: { outbound: 0, inbound: ? }
- JOBU: { outbound: 0, inbound: ? }
- Wildcard { outbound: 0, inbound: ? }
- Line 2: Account:
- XRD { outbound: 1, inbound: 0 }
- Line 3: Parent:
- XRD: { outbound: 0, inbound: ? }
- JOBU: { outbound: 0, inbound: ? }
- Wildcard { outbound: 0, inbound: ? }
- Line 5: Account
- XRD: { outbound: 5, inbound: 0 }
- Line 6: Account
- Jobu: { outbound: 2, inbound: 2 }
- Line 9: Parent:
- XRD: { sent: Inbound[L0] + Inbound[L3] - 6, received: 0 }
- Jobu: { sent: Inbound[L0] + Inbound[L3], received: 0 }
- Wildcard: { sent: Inbound[L0] + Inbound[L3], received: 0 }
There are also dynamic requirements we could output, in case the aggregator has control over these:
- Line 2: Requires that Account X accepts XRD deposits
- Line 5: Requires that Account Y has owner auth present (via an ALLOW_ALL or an allowance)
- Line 7: Requires that Account Z refunds Jobu deposits
The only branching logic we have I think is the "refund" case, but there may be others, otherwise this is all O(ResourceCount * InstructionCount^2). We would need to ditch out if we branch too many times to avoid malicious work explosion (backtracking regex vulns). e.g. naively solving this gives a solution space of 100C50 ~ O(2^N):
- Receive 100 XRD
- 100 lines of try_deposit_or_refund 1 XRD into random accounts
- Deposit 50 XRD into my account
There was a problem hiding this comment.
(The framework for this has been implemented in #1931 - it just needs a little tweak to output the constraints, but they're basically there, ready for output)
| account_deposits: IndexMap<ComponentAddress, Vec<AccountDeposit>>, | ||
| account_withdraws: IndexMap<ComponentAddress, Vec<AccountWithdraw>>, | ||
| /// The resource content of the worktop. | ||
| worktop_fungible_contents: IndexMap<FungibleResourceAddress, FungibleBounds>, |
There was a problem hiding this comment.
It's a little unexpected that worktop we separate by fungible/non-fungible, but we don't for buckets.
Not a big deal, but it might make sense to combine fungible/non-fungible into one list?
There was a problem hiding this comment.
Having the bucket contents not be separated into two state fields for fungible and non-fungible made the implementation much simpler. Also, I wanted to avoid the case where I needed to lookup the bucket contents in two of the state fields when presented with a ManifestBucket because we don't know if it's fungible or not.
There was a problem hiding this comment.
Yup, I was suggesting we have worktop_contents: IndexMap<ResourceAddress, ResourceBounds>, like buckets :). But very minor.
There was a problem hiding this comment.
It is primarily because of the logic that we use to add resources to the worktop.
Say that we store the worktop content as the following:
type WorktopContent = IndexMap<ResourceAddress, ResourceBounds>;
pub enum ResourceBounds {
Fungible(ResourceAddress, FungibleBounds),
NonFungible(ResourceAddress, NonFungibleBounds),
}If we have invocations return ResourceBounds then the logic to add the invocation output to the worktop content will look something like the following:
match (worktop_content, other) {
(Self::Fungible { .. }, Self::Fungible { .. }) => todo!(),
(Self::NonFungible { .. }, Self::NonFungible { .. }) => todo!(),
// PROBLEM:
(Self::Fungible { .. }, Self::NonFungible { .. }) => todo!(),
(Self::NonFungible { .. }, Self::Fungible { .. }) => todo!(),
}My main issue is the final two arms of the match statement which we have to make unreachable!() if we use this pattern since we can't combine fungible and non-fungible resource bounds together as a resource can't be both at the same time.
This separation of fungible content and non-fungible content avoids the above.
radix-transactions/src/manifest/static_resource_movements_visitor.rs
Outdated
Show resolved
Hide resolved
radix-transactions/src/manifest/static_resource_movements_visitor.rs
Outdated
Show resolved
Hide resolved
| TraversalEvent::End => break, | ||
| } | ||
| } | ||
| (buckets, expressions) |
There was a problem hiding this comment.
I'm a little slightly surprised you had to re-do the traversals here, rather than:
OnConsumeBucket:- Add the bucket to a list of outbound buckets this instruction
OnPassExpression- If it's worktop, add a note that whole worktop has been sent this instruction.
OnEndInstruction:- Finalize the outbound resources, and add them to a tally under the recipient kind (e.g. Parent / Child / Some Entity / Burned etc), and update the worktop bound if necessary.
- Match on the given invocation kind and scope the inbound resources
(And of course the other ones too which you handle:)
OnNewBucket:- As you do now, move resource bounds around
OnWorktopAssertion:- As you do now, adjust resource bounds
There was a problem hiding this comment.
I think that it can be implemented in the way that you described. I found this to be simpler and it keeps all of the handling of the bucket movement, expressions, and everything else in one place.
There was a problem hiding this comment.
(Demonstrated in #1931, the core visitor code is down to ~300 lines with the change, have a look and see what you think)
There was a problem hiding this comment.
Yup, I know that it can be done with the OnEndInstruction. To me it seems like different ways of doing the same thing. I do not necessarily think that one is better than the other.
| .get(&bucket) | ||
| .cloned() | ||
| .map(InvocationIo::from) | ||
| .ok_or(StaticResourceMovementsError::BucketDoesntExist(bucket)) |
There was a problem hiding this comment.
If you move this logic to OnEndInstruction rather than OnStartInstruction then you don't have to worry about things like invalid buckets and stuff, because the validator will already have taken care of it.
In fact, I'm almost tempted to make OnStartInstruction only have an index, and advise people to use OnEndInstruction instead for this reason?
There was a problem hiding this comment.
I think that we can only move this logic to OnEndInstruction if we no longer do traversal and rely on the bucket consumption callbacks and the expression callbacks. I would personally prefer not to switch to that if we don't see a massive advantage to doing it. So, I'd personally want us to keep OnStartInstruction the same as it is right now.
There was a problem hiding this comment.
I'm not sure I follow why it's not possible? OnEndInstruction has the same details on it as OnStartInstruction
There was a problem hiding this comment.
Essentially what I'm trying to say is that if we move this logic to OnEndInstruction we would have to keep track of the buckets that were consumed in the instruction as you recommended here.
radix-transactions/src/manifest/static_resource_movements_visitor.rs
Outdated
Show resolved
Hide resolved
radix-transactions/src/manifest/static_resource_movements_visitor.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Really nice work. The separation across different files has made it a lot easier to review 💯.
My comments are quite numerous, and spread across a few areas:
- Comments on code style
- Incorrect native invocation matching, inaccurate typings, and incomplete invocation listing
- Incorrect bound adjustment logic, including a number of places where we attempt to recover instead of error when impossible bounds are discovered.
- Suggestions for remodelling to reduce possibility of errors (essentially introducing simpler abstractions, separated into very thin layers, where each layer can be handled / validated separately)
Possibly it's worth us pairing on this tomorrow and we can bash out the tweaks to the models together in an hour or two?
| @@ -174,6 +174,9 @@ impl<'a, M: ReadableManifest> StaticManifestInterpreter<'a, M> { | |||
| Effect::WorktopAssertion { assertion } => { | |||
| visitor.on_worktop_assertion(OnWorktopAssertion { assertion })?; | |||
| } | |||
| Effect::Verification { verification } => { | |||
| visitor.on_verification(OnVerification { kind: verification })?; | |||
There was a problem hiding this comment.
Heads up, when you pull in develop, you're going to get compile conflicts - to maintain the behaviour, you'll need to move the "is subintent" check from handle_invocation probably to a new self.handle_verification method:
if !self.manifest.is_subintent() {
return ControlFlow::Break(
ManifestValidationError::InstructionNotSupportedInTransactionIntent.into(),
);
}| instruction_index: usize, | ||
| ) -> Vec<InvocationIo> { | ||
| vec![InvocationIo::Unknown( | ||
| WorktopUncertaintySource::Invocation { instruction_index }, |
There was a problem hiding this comment.
Taking a look at the code, it returns an owner badge, so we can return an amount of 1 of that badge.
(I guess this is a function call, so we don't know the node id of the account yet).
There was a problem hiding this comment.
Ah yes, we could return an amount of 1 with unknown ids. Seems like a good idea.
| use radix_engine_interface::blueprints::package::*; | ||
|
|
||
| pub trait StaticInvocationResourcesOutput { | ||
| fn output( |
There was a problem hiding this comment.
This interface feels slightly off - initially I thought that we don't necessarily need to change it now, BUT after reviewing this whole PR, I think we need to simplify/consolidate our abstractions to decrease the conceptual complexity and make it easier to avoid bugs or regressions.
My couple of minor reflections are:
- It's weird that they need to pass the instruction index - this could be handled in the layer above to turn it into uncertainty.
- Having a
Vec<InvocationIo>feels a little overlapping perhaps? - (Returning to this later) - in general, these APIs give too much access into their internals, which let `` shoot ourselves in the foot
Something like this modelling feels more semantic, simpler to reason about, less buggy, and more powerful:
pub trait StaticInvocationResourcesOutput {
fn output(&self, details: InvocationDetails) -> ResourceBounds {
ResourceBounds::new_empty()
}
}
pub struct InvocationDetails {
receiver: Option<GlobalAddress>,
sent_resources: ResourceBounds,
source: ChangeSource,
}
/// Used for the worktop, and for any instruction input/output.
#[derive(Default)]
pub struct ResourceBounds {
bounds: IndexMap<ResourceAddress, ResourceBound>,
unspecified_resource_sources: Vec<ChangeSource>,
}
impl ResourceBounds {
// Constructors
pub fn new_empty() -> Self; // unspecified_resources_possible: false
pub fn new_including_unspecified_resources(source: ResourceChangeSource) -> Self;
// Deconstructors
pub fn into_known_bounds(self) -> IndexMap<ResourceAddress, ResourceBound>;
pub fn deconstruct(self) -> (IndexMap<ResourceAddress, ResourceBound>, Vec<ChangeSource>);
// &self Methods
pub fn known_bounds(&self) -> &IndexMap<ResourceAddress, ResourceBound>;
pub fn resource_bound(&self, resource: ResourceAddress, request_source: ChangeSource) -> ResourceBound {
// If in list, return that.
// If not, and we have unspecified_resource_sources, create it as any with the given ChangeSources
// If not, and we have no unspecified_resource_sources, create it as none with the given request_source
}
pub fn can_include_unspecified_resources(&self) -> bool;
pub fn unspecified_resource_sources(&self) -> &[ChangeSource];
// &mut self Methods (check that resource bound aligns with resource type)
fn resource_bound_mut(&mut self, resource: ResourceAddress, source: ChangeSource) -> &mut ResourceBound {
// The change source is just used for creating the resource bound if it doesn't exist already
// Creates if not exist using resource_bound, and stores it.
}
pub fn add(&mut self, resources: ResourceBounds, source: ChangeSource) -> Result<(), Error> {
// Merges the two
}
fn add_resource(&mut self, resource: ResourceAddress, amount: ResourceBound) -> Result<(), Error>
{
self.resource_bound_mut(resource).add_from(amount)
}
pub fn take_resource(&mut self, resource: ResourceAddress, amount: ResourceTakeAmount, source: ChangeSource) -> Result<ResourceBound, Error> {
self.resource_bound_mut(resource).take(amount, source)
}
pub fn take_all(&mut self) -> Self {
core::mem::take(self)
}
pub fn handle_worktop_assertion(worktop_assertion: WorktopAssertion) -> Result<(), Error> {
// ... calls to self.handle_resource_assertion(resource, assertion) where appropriate
// FUTURE TWEAK: Could return an optional set of constraints using all_changes
}
fn handle_resource_assertion(&mut self, resource: ResourceAddress, assertion: ResourceAssertion) -> Result<(), Error> {
self.resource_bound_mut(resource).handle_assertion(constraint)
}
}
pub enum ResourceTakeAmount {
Amount(Decimal),
NonFungibles(IndexSet<NonFungibleLocalId>),
All,
}
impl ResourceTakeAmount {
// Helper constructors for prettier code
pub fn non_fungibles(ids: impl IntoIterator<Item = NonFungibleLocalId>) -> Self;
pub fn amount(amount: impl ResolvableDecimal) -> Self;
pub fn all() -> Self;
}
/// Used to track a known quantity of Fungible and NonFungible resources,
/// for example, the content of a bucket.
pub struct ResourceBound {
lower_inclusive: Decimal,
upper_inclusive: Decimal, // Unbounded = Decimal::MAX and we can use saturating add when adding upper bounds.
/// A maintained invariant is that the number of known ids must be <= the upper bound.
/// Any take by amount will wipe these, because we don't know which will get taken.
known_ids: IndexSet<NonFungibleLocalId>,
history: ResourceChangeHistory,
}
impl ResourceBound {
// Constructors (check decimals are > 0)
pub fn zero(source: ChangeSource) -> Self;
pub fn exact(amount: impl ResolvableDecimal, source: ChangeSource) -> Self;
pub fn at_least(amount: impl ResolvableDecimal, source: ChangeSource) -> Self;
pub fn non_zero(source: ChangeSource) -> Self; // Self::at_least(source, Decimal::MIN_POSITIVE_VALUE)
pub fn any(source: ChangeSource) -> Self; // Self::at_least(source, 0);
pub fn any_from_sources(sources: impl IntoIterator<Item = ChangeSource>) -> Self;
pub fn non_fungibles(ids: impl IntoIterator<Item = NonFungibleLocalId>, source: ChangeSource) -> Self;
// &self methods
pub fn inclusive_bounds(&self) -> (Decimal, Decimal);
pub fn known_ids(&self) -> &IndexSet<NonFungibleLocalId>;
pub fn history(&self) -> &ResourceChangeHistory;
// &mut self methods.
pub fn add_from(&mut self, existing: ResourceBound) -> Result<(), Error> {
// Errors if Fungible resource used with NFs
// Merge with current
history.add_from(existing);
}
pub fn take(&mut self, amount: ResourceTakeAmount, source: ChangeSource) -> Result<ResourceBound, Error> {
// Errors if fungible resource used with NFs
// Errors if invariants are broken, e.g. lower > upper
history.take(amount, source);
// FUTURE TWEAK: Could also yield a constraint using all_changes
}
pub fn take_all(&mut self, source: ChangeSource) -> Self {
core::mem::replace(self, Self::zero(source))
}
pub fn handle_assertion(&mut self, assertion: ResourceAssertion) -> Result<(), Error> {
// Errors if invariants are broken, e.g. lower > upper, or known_ids().len() > upper
// FUTURE TWEAK: Could also yield a constraint using history.all_changes(),
// ... representing what's required in history for this assertion to succeed.
}
}
pub struct ResourceAssertion {
lower_inclusive: Decimal,
upper_inclusive: Decimal,
required_ids: IndexSet<NonFungibleLocalId>,
}
impl ResourceAssertion {
pub fn any() -> Self; // Self::at_least(source, 0);
pub fn exact_amount(amount: impl ResolvableDecimal) -> Self;
pub fn at_least_amount(amount: impl ResolvableDecimal) -> Self;
pub fn non_zero_amount() -> Self; // Self::at_least(source, Decimal::MIN_POSITIVE_VALUE)
pub fn exact_non_fungibles(ids: impl IntoIterator<Item = NonFungibleLocalId>) -> Self;
pub fn at_least_non_fungibles(ids: impl IntoIterator<Item = NonFungibleLocalId>) -> Self;
pub fn general(lower_inclusive: Decimal, upper_inclusive: Decimal, required_ids: impl IntoIterator<Item = NonFungibleLocalId>) -> Self;
}
pub struct ResourceChangeHistory(Vec<ResourceChange>);
impl ResourceChangeHistory {
pub fn initial_empty(source: ChangeSource) -> Self;
pub fn initial_from(sources: impl IntoIterator<Item = ChangeSource>) -> Self; // Used for creating resource entries from an unknown quantity
pub fn initial_add(amount: ResourceAddAmount, source: ChangeSource) -> Self;
pub fn take(&mut self, amount: ResourceAddAmount, source: ChangeSource);
pub fn add_from(&mut self, bounds: ResourceBounds);
/// Can be used to calculate requirements which must be upheld during `take` or `handle_assertion`
pub fn changes(&self) -> impl Iterator<Item = ResourceChange>;
pub fn all_changes(&self) -> impl Iterator<Item = SimpleResourceChange>;
}
pub enum SimpleResourceChange {
Add(ResourceAddAmount, ChangeSource),
Take(ResourceTakeAmount, ChangeSource),
}
pub struct ResourceAddAmount {
lower_inclusive: Decimal,
upper_inclusive: Decimal, // Unbounded = Decimal::MAX and we can use saturating add when adding upper bounds.
/// A maintained invariant is that the number of known ids must be <= the upper bound.
/// Any take by amount will wipe these, because we don't know which will get taken.
known_ids: IndexSet<NonFungibleLocalId>,
}
enum ResourceChange {
InitialAdd(ResourceAddAmount, ChangeSource),
Take(ResourceTakeAmount, ChangeSource),
/// Internally includes a history of changes sources. This can be flattened by the `all_changes` method.
ExistingAdd(ResourceBound),
}
/// Created by the visitor, generally references a particular instruction, or maybe an initial YIELD_TO_PARENT.
#[derive(Debug, Clone, Copy)]
pub struct ChangeSource {
// To avoid lots of duplication, it makes sense for this to be an index into a separate ActualChangeSourceList, owned by the visitor.
// This is then a list of ActualChangeSource { instruction_index: Option<usize>, invocation_kind: InvocationKind };
change_source_index: usize,
}And we could construct bounds like:
ResourceBounds::only()
.add(resource, ResourceBound::exact(1, source))
.add(other_resource, ResourceBound::non_fungibles([id1], source))
ResourceBounds::includes_unspecified_resources(source)
.add(resource, ResourceBound::exact(1, source))There was a problem hiding this comment.
It's weird that they need to pass the instruction index - this could be handled in the layer above to turn it into uncertainty.
The client doesn't need to pass in an instruction index. Not entirely sure what this means?
Having a Vec feels a little overlapping perhaps?
In what way?
(Returning to this later) - in general, these APIs give too much access into their internals, which let `` shoot ourselves in the foot
I think examples of this would be useful. I think that we provide clients with a good amount of information that they can then use to do many things. So, I would like more information on this concern.
Looking at the code block above, I can see that the interface is nicer. However, I'm still not clear on why we need to make this change.
There was a problem hiding this comment.
The client doesn't need to pass in an instruction index. Not entirely sure what this means?
The API for output mentions a node_id and instruction_index. I've changed this in 1931 to a parameter object refactor to avoid the method signature of output from being so long; and encapsulated instruction_index into a change_source object.
| impl StaticInvocationResourcesOutput for AccountSecurifyInput { | ||
| fn output( | ||
| &self, | ||
| node_id: &NodeId, |
There was a problem hiding this comment.
How do we handle node_id not being present, say, because it was a reserved address?
In that case I guess we can return ACCOUNT_OWNER_BADGE with Amount of 1? Rather than a specific one?
There was a problem hiding this comment.
Reserved address handling still needs implementing :)
There was a problem hiding this comment.
Yeah we do not handle reserved addresses at all at the moment, even though we might be able to, but it adds complexity. Currently, if the invocation happens to a non-static address we're unable to turn it into a typed invocation and just say that the invocation returns unknown amounts.
| // Convert to a composite resource address. | ||
| let composite_resource_address = CompositeResourceAddress::from(*resource_address); | ||
|
|
||
| match (composite_resource_address, assertion) { |
There was a problem hiding this comment.
MINOR: It would be nice if this was a handle_resource_assertion(resource_address, resource_bound) on worktop = FullResourceBounds to put the logic in one place.
This code could be a lot shorter and make it harder to make mistakes if we tweak our abstractions :).
| WorktopAssertion::AnyAmountGreaterThanZero { .. }, | ||
| ) => { | ||
| self.worktop_fungible_contents | ||
| .entry(fungible_resource_address) |
There was a problem hiding this comment.
This isn't quite right - if it exists, we should handle the bound, if not, we should set to the bound.
e.g. if XRD = ExactAmount(0) then this assertion should change it to NonZero.
There was a problem hiding this comment.
Good point, didn't consider this case.
| // fungible ids. The logic is going to depend on the state of the non-fungible | ||
| // bounds. | ||
| match non_fungible_contents.id_bounds { | ||
| // If they're fully known and an assertion comes with ids outside of the |
| } | ||
| } | ||
| (_, WorktopAssertion::IsEmpty) => { | ||
| // Empty the worktop completely. |
There was a problem hiding this comment.
We've already handled this above, so this is unreachable.
| .lock_fee_and_withdraw(account1, 100, XRD, 10) | ||
| .take_from_worktop(XRD, 10, "bucket") | ||
| .deposit(account2, "bucket") | ||
| .build(); |
There was a problem hiding this comment.
Side note - if 15 XRD was withdrawn, this should be an error because there is a non-zero lower bound of a resource left on the worktop at the end.
…ovements-static-analysis
Summary
This PR adds an analyzer for the resource movements in the manifest.