-
Notifications
You must be signed in to change notification settings - Fork 121
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.
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
Feature/assert next call instructions #1958
Conversation
…t-call-instructions
…t-call-instructions
…t-call-instructions
Docker tags |
Benchmark for 2d5e7c9Click to view benchmark
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Happy to merge subject to discussion of comments 👍
Ok(()) | ||
} | ||
} | ||
|
||
struct ResourceConstraintChecker { | ||
fungible_resources: BTreeMap<ResourceAddress, Decimal>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default, can we use IndexMap unless we have to use an invariant ordering?
IndexMap
is more efficient than BTreeMap, and less surprising when things get arbitrarily ordered by byte-ordering of resource address.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was originally IndexMap
but changed it to use BTreeMap
since I want the ability to remove from a map without using swap_remove
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing wrong with swap_remove
, it's just remove which explains how it affects the ordering.
But yeah, NonIterMap
was designed for this purpose. Or if we need to iter, maybe there's some other way to do the algorithm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good point, will switch back to IndexMap
objects: &mut IntentProcessorObjects, | ||
_api: &mut Y, | ||
) -> Result<InstructionOutput, RuntimeError> { | ||
objects.next_call_return_constraints = Some(NextCallReturnConstraints { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency, perhaps this should be next_call_returns_constraints = Some(NextCallReturnsConstraints
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should panic or throw an error if this is already set when we set it?
It shouldn't be possible (the validation should enforce it) but sometimes we run transactions without validation (e.g. system transactions).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine behavior that the system will just override the last setting.
let ids = non_fungible_resources | ||
.remove(&resource_address) | ||
.unwrap_or_default() | ||
.clone(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the clone is unneeded?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch
radix-common/src/data/manifest/model/manifest_resource_assertion.rs
Outdated
Show resolved
Hide resolved
} | ||
|
||
if exact { | ||
if !fungible_resources.is_empty() || !non_fungible_resources.is_empty() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only correct if these are all non-zero balances. If this is required, it should IMO be expressed in the parameter name or rust doc. Currently I think this invariant is correctly upheld inside the instruction processor, but it's a little indirect for my liking.
Instead, perhaps we could just iterate across the remaining fungible_resources
and non_fungible_resources
and if any of the balances are non-zero, then return an error with an example resource? That would then remove this pre-condition; and also allow us to give an example resource which was present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I think we should add a test for this (to avoid potential regressions in future)
e.g.
ASSERT_NEXT_CALL_RETURNS_EXACTLY <empty>
and thenwithdraw XRD Decimal("0")
should succeedASSERT_NEXT_CALL_RETURNS_EXACTLY <empty>
and thenwithdraw <nfs> <empty>
should succeed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, for good regression coverage, would be nice to have a quick test which applies an unmeetable ASSERT_NEXT_CALL_RETURNS_EXACTLY
before a CALL_FUNCTION
and both YIELD_
commands (and e.g. check it gives a failure)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only correct if these are all non-zero balances. If this is required, it should IMO be expressed in the parameter name or rust doc. Currently I think this invariant is correctly upheld inside the instruction processor, but it's a little indirect for my liking.
Instead, perhaps we could just iterate across the remaining
fungible_resources
andnon_fungible_resources
and if any of the balances are non-zero, then return an error with an example resource? That would then remove this pre-condition; and also allow us to give an example resource which was present.
Good point will fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, for good regression coverage, would be nice to have a quick test which applies an unmeetable
ASSERT_NEXT_CALL_RETURNS_EXACTLY
before aCALL_FUNCTION
and bothYIELD_
commands (and e.g. check it gives a failure)
This is already covered by tests when_less_is_returned_assert_next_call_returns_only_should_fail
and when_more_is_returned_assert_next_call_returns_only_should_fail
though it's missing the YIELD_TO_CHILD
command. Will add this.
e243686
to
f4db6d6
Compare
ASSERT_NEXT_CALL_RETURNS_ONLY
andASSERT_NEXT_CALL_RETURNS_INCLUDE
instructions