-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Use generic queries in the VM #2736
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
Use generic queries in the VM #2736
Conversation
af2f919
to
3f795bd
Compare
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.
LGTM. This will break a ton of dependent software (tx-cannons, delegated provers). I'll make sure to notify everyone during the canary release.
ledger/benches/transaction.rs
Outdated
@@ -122,15 +142,20 @@ fn execute(c: &mut Criterion) { | |||
vm.execute_authorization( | |||
execute_authorization.replicate(), | |||
Some(fee_authorization.replicate()), | |||
None, | |||
None::<Query<MainnetV0, <LedgerType<_> as ConsensusStorage<_>>::BlockStorage>>, |
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.
Should we add a helper function that returns None
or the empty query from below?
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'm not sure if that's possible (due to concrete/generic type as output), but this gave me the idea that I could introduce type aliases to greatly reduce the diff.
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.
Looks good! I added two suggestions.
26b1325
to
0fe2c9c
Compare
Signed-off-by: ljedrz <ljedrz@users.noreply.github.com>
Signed-off-by: ljedrz <ljedrz@users.noreply.github.com>
Signed-off-by: ljedrz <ljedrz@users.noreply.github.com>
Signed-off-by: ljedrz <ljedrz@users.noreply.github.com>
Signed-off-by: ljedrz <ljedrz@users.noreply.github.com>
c7d2006
to
24845db
Compare
Caveats:
QueryTrait
now requires that the implementors areClone
None
), it gets recreated in functions that need to use it more than once; this is perfectly fine with the currentQuery
object, or if we assume the query to typically be providedThe 1st commit is the actual change, and the related diff is small; the second commit, and the vast majority of the PR's diff, is the formatting fallout from adjusting the tests to disambiguate the
None
values.