Conversation
juan518munoz
added
the
no changelog
igamigo
left a comment
igamigo
left a comment
There was a problem hiding this comment.
LGTM! Left some comments, but overall looks good.
igamigo
left a comment
igamigo
left a comment
There was a problem hiding this comment.
Left a couple more comments, but I believe we can merge after them.
|
|
||
| #[cfg(feature = "tonic")] | ||
| impl proto::rpc_store::account_proof::AccountDetailsResponse { | ||
| impl proto::rpc_store::account_proof_response::AccountDetails { |
There was a problem hiding this comment.
nit: can we use namespaces for the imports since this file is not generated?
There was a problem hiding this comment.
Note that pub struct AccountDetails is defined on the file, so we would be redefining the type AccountDetails.
A possible alternative would be to:
use proto::rpc_store::account_proof_response::AccountDetails as ProtoAccountDetailsBut doing so might break cohesion with the rest of the codebase.
| map_key: Word, | ||
| ) -> Result<miden_objects::account::StorageMapWitness, DataStoreError> { | ||
| //TODO: Refactor the store call to be able to retrieve by map root. | ||
| // TODO: Refactor the store call to be able to retrieve by map root. |
There was a problem hiding this comment.
Do we have an issue for this?
There was a problem hiding this comment.
It seems as there is no issue to tracking this, should we add it?
There was a problem hiding this comment.
I'll create one in a few minutes.
fkrause98
left a comment
fkrause98
left a comment
There was a problem hiding this comment.
left some minor comments
3 participants
Note: there still some todo's left to address before opening this PR for review.
Depends on 0xMiden/node#1300