Feature/resource movements static analysis #1909
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
…to feature/resource-movements-static-analysis
|
Docker tags |
Benchmark for e275fc7Click to view benchmark
|
0xOmarA
changed the base branch from
feature/subintent-validation
to
develop
dhedey
reviewed
Sep 19, 2024
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 :)
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
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
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
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
dhedey
requested changes
Sep 26, 2024
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?
radix-transactions/src/manifest/static_resource_movements/effect.rs
Outdated
Show resolved
Hide resolved
radix-transactions/src/manifest/static_resource_movements/effect.rs
Outdated
Show resolved
Hide resolved
radix-transactions/src/manifest/static_resource_movements/effect.rs
Outdated
Show resolved
Hide resolved
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
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
radix-engine-tests/tests/application/static_resource_movements_visitor.rs
Show resolved
Hide resolved
* It errors if it can't encode/decode, or can't match the native method/function * Named / reserved addresses work
…ovements-static-analysis
…tic-analysis-rework rework: Rework the manifest resource interpreter
dhedey
approved these changes
Oct 2, 2024
There was a problem hiding this comment.
I'm happy to get this merged (not least to unblock my new manifest instruction PR) and to do the follow ups in a separate PR :)
The follow-ups I think are detailed in:
- Comments on this PR: rework: Rework the manifest resource interpreter #1931
- A few oustanding comments on this PR
And notably:
- Complete defining the packages
- Possibly move the type native invocations to the interfaces crate
- Add a test which iterates over every blueprint definition in post-latest DB, and:
- Ensures Completeness: Checks that resolving a native invocation with () results in either a success or a "CantDecode" error.
- Ensures Schema Accuracy: Checks that resolving a schema for the native invocation aligns with the type from the Database.
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.
Summary
This PR adds an analyzer for the resource movements in the manifest.