feat: offline wallet creation & various fixes #2061
Conversation
There was a problem hiding this comment.
great work
i havent looked at how a zingo-cli user should set the server to offline mode. i also havent looked thouroughly at how zingolib consumers such as zingo-mobile would set to serverless but my assumption is they can directly create a config with URI set to None. i will look into these things on my final review
|
|
||
| let indexer_uri: Option<http::Uri> = match indexer_uri_str { | ||
| Some(s) => Some(zingolib::config::parse_indexer_uri(s)?), | ||
| None => None, |
There was a problem hiding this comment.
minor:
a map would do nicely here
| match chain_name { | ||
| "testnet" => Ok(ChainType::Testnet), | ||
| "mainnet" => Ok(ChainType::Mainnet), | ||
| // TODO: FIXME |
There was a problem hiding this comment.
fixme? also i see these NU heights more than once, maybe we could create a const to DRY
There was a problem hiding this comment.
Yeah. I was thinking that these activation heights shouldn't be hard-coded. Maybe the desired regtest has a different set of activation heights.
One way is to get them from the node itself, though that will require some work on zingo-cli. Either that or allowing the user to define a config, but I'd like to avoid creating a file just for that parameter.
| serde_json = { workspace = true } | ||
| tempfile = { workspace = true, optional = true } | ||
| thiserror = { workspace = true } | ||
| thiserror.workspace = true |
There was a problem hiding this comment.
i think that we should either leavev this or change all of them. consistency is the key thing here
| #[folder = "zcash-params/"] | ||
| pub struct SaplingParams; | ||
|
|
||
| pub use zebra_chain::parameters::testnet::ConfiguredActivationHeights; |
| } | ||
| s | ||
| } | ||
| // TODO: Is this really necessary? |
There was a problem hiding this comment.
I think defaulting to port 9067 is useful as this is the standard port. im not sure about adding http prefix or checking for empty now you have fixed the offfline logic
There was a problem hiding this comment.
we should also handle the error instead of unwrap. maybe we can clean this up
| } | ||
|
|
||
| /// TODO: Add Doc Comment Here! | ||
| /// Builds a [`ZingoConfig`]. If no indexer is set, it's interpreted as offline. |
There was a problem hiding this comment.
isnt it offline when URI is none? the unwrap_or_default followed by Some(lightwalletd_uri) means that it cant be interpreted as offline right?
There was a problem hiding this comment.
You're right, this was a mistake from before! I'll update this.
| if self.indexer_uri.read().unwrap().is_some() { | ||
| return Some( | ||
| self.indexer_uri | ||
| .as_ref() | ||
| .read() | ||
| .unwrap() | ||
| .clone() | ||
| .expect("Couldn't read configured server URI!") | ||
| .clone(), | ||
| ); | ||
| } | ||
| None |
There was a problem hiding this comment.
| if self.indexer_uri.read().unwrap().is_some() { | |
| return Some( | |
| self.indexer_uri | |
| .as_ref() | |
| .read() | |
| .unwrap() | |
| .clone() | |
| .expect("Couldn't read configured server URI!") | |
| .clone(), | |
| ); | |
| } | |
| None | |
| self.indexer_uri.read().expect("Couldn't read configured server URI!").map(|uri| { | |
| uri.clone() | |
| }) |
There was a problem hiding this comment.
untested but you get the idea
| let valid_uri = crate::config::parse_indexer_uri( | ||
| crate::config::DEFAULT_LIGHTWALLETD_SERVER.to_string(), | ||
| )); | ||
| ) |
There was a problem hiding this comment.
it seems you have removed a load config test that should contain this URI (and incorrectly tested serverless previously) but now have added a serverless test which creates a config wtih a server?
can we call this test load_clientconfig and duplcate it and call it load_clientconfig_serverless with a None in the URI field and change the assertions accordingly?
|
oh also the README will need updating if you ahve removed the regest feature flag from zingo-cli |
Co-authored-by: oscar-pepper <109323234+Oscar-Pepper@users.noreply.github.com> Signed-off-by: Dorian <58955380+dorianvp@users.noreply.github.com>
No description provided.