-
Notifications
You must be signed in to change notification settings - Fork 956
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
Murisi/masp separate parallel sync #2474
Conversation
…ization function.
fn parse(matches: &ArgMatches) -> Self { | ||
let ledger_address = LEDGER_ADDRESS_DEFAULT.parse(matches); | ||
let last_query_height = BLOCK_HEIGHT_OPT.parse(matches); | ||
let viewing_keys = VIEWING_KEYS.parse(matches); |
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.
do we not want to support passing in spending keys 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.
Whenever a spending key is added to the wallet, its corresponding viewing key is also added under the same name. So I'm thinking that that this code should be sufficient for now. No?
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.
Sure, I just wondered if we considered it important.
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.
A couple of small changes. Also, I'm not seeing marco's changes in here. I think there is a divergence between base and main.
crates/apps/src/lib/cli/client.rs
Outdated
.collect::<Vec<_>>(); | ||
let mut shielded = namada.shielded_mut().await; | ||
let _ = shielded.load().await; | ||
for vk in wallet_keys { |
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.
Let's leave the internals of vk heights inside of the sync
function. Instead lets concatenate all of the keys together and pass them into the shielded sync function.
} | ||
// the latest block height which has been added to the witness Merkle |
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 actually surprised that the integration tests pass. There is bug here that one failed for me on. We need to save the shielded context here to persist the keys.
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 to me, thank you!
Describe your changes
This PR creates a separate synchronization command and heavily draws code from #2422 . More specifically, this PR's changes are as follows:
shielded-sync
shielded-sync
s before doing any actionsIndicate on which release or other PRs this topic is based on
#2458
Checklist before merging to
draft