Refactor Manifest Classification #132
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This reverts commit 5e6109a.
This reverts commit 13ee650.
Add a general subintent tx type & make owner key changes reserved
Added a test for manifests with yield to child
The old `models` module was barely getting used despite the toolkit having a lot of types that it uses. Additionally, the types were spread across different modules such as `sbor` and `models`. This commit moves all of the types from these two modules into a new module called `types` which is where the majority of the toolkit types will live from now on.
This commit added `prelude` and `internal_prelude` modules to the toolkit to ease the imports a little bit as the imports were getting a bit out of hand in some of the modules. The `prelude` module is meant to be public and usable by all clients of the toolkit whereas the `internal_prelude` is meant to be internal to the toolkit itself and only usable by it.
This commit is the start of a move of certain utility functions and methods from the `utils` module and into a new module called the `extensions` module. Extensions allow us to define more functions and methods on types almost as if the types are defined in our crate, they do this by using new traits. Eventually, in this PR, there should no longer be a utils module and there should only be an extensions module.
This commit adds a new `GroupedInstruction` type which is defined in the `types::grouped_instruction` module alongside the macro that defines it. Everything about this newly added type is defined through a macro which allows us to not write or maintain the 1600 lines of code that get generated by the macro.
In a similar way to instructions, grouping entity types can be useful in transaction type classification and therefore we introduce a new concept of grouped entity types. This concept should allow for a simple way to do checks such as "is the entity an account" instead of needing to check against multiple different entity types.
This commit adds an extension to the `ResourceSpecifier` type that adds some convince methods to the type such as methods for getting the resource address, amount, ids, and checking if the `ResourceSpecifier` is empty.
This commit adds a `ManifestAnalysisVisitor` trait which is going to be used in the manifest analysis heavily for classification and retrieval of data from manifests and in interpreting instructions.
This commit introduces the very first visitor for manifest analysis: the presented proofs visitor. This visitor has single simple job: find all instances of a call to the account's create proof methods and record the resource address and amount of the created proofs.
This commit adds a visitor whose main responsibility is to extract the set of accounts and identities that the wallet needs to provide auth for whether it be signatures or access controller proofs in the future when MFA is released.
Added a visitor for reserved instructions to detect them.
This commit adds a visitor which detects all of the account interactions made in a manifest and surfaces them to the client.
This commit adds a way for defining composite visitors that do not use dynamic dispatch. This is helpful in getting rid of the need for the nightly compiler in the toolkit.
This commit renames the `VisitorValidityState` trait to be `ManifestAnalysisVisitorValidityState` which is more inline with the trait name.
This commit updates the uniffi crate for the latest changes made in the upstream RET crate.
This commit updates the Scrypto dependency to one that has the missing methods in the `TypedManifestNativeInvocation` type and makes changes to the affected areas of the code.
dhedey
reviewed
Jan 22, 2025
crates/radix-engine-toolkit/src/manifest_analysis/types/static_analysis.rs
Show resolved
Hide resolved
This is a small commit that's introducing a new type that captures both the receiver and the manifest typed invocation into a single type that we can supply to visitors.
Updated the Scrypto dependency to the latest cuttlefish version
This commit switches the visitor arguments from being separate arguments and into a single argument called `AnalysisContext` that contains references to the information that the visitor might find to be useful.
The requirement state trait now requires that a `RequirementState` enum is returned when checking if the requirements are fulfilled. This allows for more granular description of the requirement state and whether the it can be fulfilled in the future or not.
This commit adds a method to the permission state trait for processing instructions. This means that the visitor is no longer responsible for processing the permission of instructions and the permission state is responsible for determining if an instruction is permitted or not.
Requirement state updates are no longer computed through the visitor and are now computed separately from it by the requirement state itself.
This commit adds the `AnyOfRequirement` and `AllOfRequirement` types which are requirement combinator types that can be used to combine various requirements together in an `and` and `or` style.
This commit changes the requirements used in the analyzers so that we're using modular and composable requirements that are allowed for by the requirement combinator.
This commit removes the requirement from dynamic analysis and aligns the requirement for dynamic analysis to be the same as static analysis.
…fest-classification
dhedey
reviewed
Jan 28, 2025
There was a problem hiding this comment.
Nice! Didn't notice any logical issues. Lots of tests which are great.
I think most/all of the comments are just a few minor code structure things that I commented on as I passed by, but mostly just personal preference. Although in some cases, we are duplicating quite a lot of code and maybe could make things less boilerplaty with some nice semantic methods.
crates/radix-engine-toolkit/src/manifest_analysis/types/analysis_context.rs
Outdated
Show resolved
Hide resolved
crates/radix-engine-toolkit/src/manifest_analysis/traits/requirement_state.rs
Outdated
Show resolved
Hide resolved
crates/radix-engine-toolkit/src/manifest_analysis/requirements/any.rs
Outdated
Show resolved
Hide resolved
| /// by the visitor. If the manifest were to end with a visitor in this state | ||
| /// then the visitor would not produce output since it's requirements were't | ||
| /// fulfilled. | ||
| CurrentlyUnfulfilled, |
There was a problem hiding this comment.
What is the purpose of this enum? It appears like this is only used to check if a requirement is fulfilled right at the end; at which point it feels a bit unnatural to return CurrentlyUnfulfilled and PermanentlyUnfulfilled as distinct possibilities.
I suggest this should actually just be these two elements:
enum RequirementOutcome {
Fulfilled,
Rejected,
}Internally, inside a requirement, we might need to handle the three states, but I think it's a minor smell to have these three states on the output of the requirement -- unless there is any need to "check on" the requirement mid-processing, but I haven't seen that need?
There was a problem hiding this comment.
It not being used was a bit of an oversight. It should be used. The idea behind it is that it's a way for the visitor to signal to the traverser that its requirements can no longer be fulfilled and that there's no point in continuing to traverse the manifest. I've updated the traverser to handle this.
There was a problem hiding this comment.
Are you saying that the traverser is checking this every instruction as a potential performance improvement? I think it's probably of questionable benefit as an optimisation, but I don't mind it.
I imagine it would simplify things to move to a final-outcome based model; but happy to leave things as-is for now :)
crates/radix-engine-toolkit/src/manifest_analysis/classification/simple_transfer.rs
Outdated
Show resolved
Hide resolved
crates/radix-engine-toolkit/src/manifest_analysis/classification/validator_claim.rs
Outdated
Show resolved
Hide resolved
crates/radix-engine-toolkit/src/manifest_analysis/classification/validator_stake.rs
Outdated
Show resolved
Hide resolved
crates/radix-engine-toolkit/src/manifest_analysis/classification/validator_unstake.rs
Outdated
Show resolved
Hide resolved
dhedey
approved these changes
Jan 28, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.