simplify ValidityPredicate trait #201
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
mariari
reviewed
Aug 23, 2023
Comment on lines
+58
to
+59
| pub type ValidityPredicate = dyn ValidityPredicateVerifyingInfo; | ||
|
|
There was a problem hiding this comment.
Yeah this is a fine simplification, seems it is still used everywhere, however this does bring up a question.
taiga_halo2/src/note.rs
Outdated
Comment on lines
99
to
100
| app_vp_verifying_info: Box<dyn ValidityPredicateVerifyingInfo>, | ||
| app_vp_verifying_info_dynamic: Vec<Box<dyn ValidityPredicateVerifyingInfo>>, | ||
| app_vp_verifying_info: Box<ValidityPredicate>, | ||
| app_vp_verifying_info_dynamic: Vec<Box<ValidityPredicate>>, |
There was a problem hiding this comment.
It seems here we are still essentially being reliant upon a dyn trait, if we were to expand this alias, it ends up being
app_vp_verifying_info: Box<dyn ValidityPredicateVerifyingInfo>
app_vp_verifying_info_dynamic: Vec,Box<dyn ValidityPredicateVerifyingInfo>>
meaning that binding to this is still hard if this is exposed as the outside interface
taiga_halo2/src/note.rs
Outdated
Comment on lines
106
to
107
| app_vp_verifying_info: Box<dyn ValidityPredicateVerifyingInfo>, | ||
| app_vp_verifying_info_dynamic: Vec<Box<dyn ValidityPredicateVerifyingInfo>>, | ||
| app_vp_verifying_info: Box<ValidityPredicate>, | ||
| app_vp_verifying_info_dynamic: Vec<Box<ValidityPredicate>>, |
| circuit::{vp_circuit::ValidityPredicateInfo, vp_examples::TrivialValidityPredicateCircuit}, | ||
| circuit::{vp_circuit::ValidityPredicateCircuit, vp_examples::TrivialValidityPredicateCircuit}, |
There was a problem hiding this comment.
Probably should have changed with the commit that removed the type,, but good that the bench still works.
XuyangSong
force-pushed
the
xuyang/simplify_vp_trait
branch
from
dad72c2
to
eb3a0e2
Compare
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.
ValidityPredicateInfotrait intoValidityPredicateCircuittrait. Can not merge theValidityPredicateVerifyingInfotrait because it causes an "unsafe object" compile error when creating a dynamic object.ValidityPredicateConfigtrait and use the concrete ValidityPredicateConfig struct instead since we've unified the VP config. TODO: fix the Sudoku example later, which uses the deprecatedValidityPredicateConfigtrait.app_vp_verifying_info/app_vp_verifying_info_dynamicin the Input/OuputNoteProvingInfo toapplication_vp/dynamic_vpsValidityPredicateCircuitof Taiga.