-
Notifications
You must be signed in to change notification settings - Fork 158
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
feat: Extend AdviceProvider
trait
#987
Conversation
2317779
to
5176881
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.
Looks good! Thank you! I left a couple of comments inline. Once these are addressed, we can merge.
core/Cargo.toml
Outdated
@@ -21,7 +21,7 @@ std = ["math/std", "winter-utils/std"] | |||
|
|||
[dependencies] | |||
math = { package = "winter-math", version = "0.6", default-features = false } | |||
crypto = { package = "miden-crypto", version = "0.6", default-features = false } | |||
crypto = { package = "miden-crypto", git = "https://github.com/0xPolygonMiden/crypto.git", branch = "frisitano-tx-result", default-features = false } |
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 might have missed it - but seems like the changes in this PR should work fine with v0.6
. If so, let's revert back here.
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.
That's correct however I needed to have consistent versions of crypto for my work in miden-base
. It's not a blocker for this PR. I will revert v0.6, merge and then restore the branch and continue using it for downstream (miden-base) dev.
processor/src/advice/providers.rs
Outdated
// TODO: Do we want to return a generic MerkleStore instead of a SimpleMerkleMap? | ||
fn get_store_subset<I, R>(&self, roots: I) -> MerkleStore |
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 haven't thought about it much. Seems like returning MerkleStore
(the way it is now) is a bit clearer - but I don't really have a strong preference.
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.
Lets leave it as is as theres no obvious reason to make it generic at this point.
5176881
to
764e8be
Compare
This PR introduces
get_mapped_values(..)
andget_store_subset(..)
methods on theAdviceProvider
trait and implements the methods forBaseAdviceProvider
,MemAdviceProvider
andRecAdviceProvider
.