Added ability to configure AMP2 client connection#143
Added ability to configure AMP2 client connection#143kevandee wants to merge 2 commits intoBlockstream:masterfrom
Conversation
|
In commit b39bc8f "Removed unnecessary constants in config", you're removing stuff that you added in your previous commit. Please clean up your commits before submitting for review. |
|
We have an internal gitlab ci, you should submit PR there (too). |
|
You're doing changes to both "wollet" and "app/cli" crate.
See git history for reference. |
|
Why are you doing the change for "app/cli", |
|
|
||
| impl Amp2 { | ||
| /// Create a new AMP2 client | ||
| pub fn new(server_key: String, url: String) -> Self { |
There was a problem hiding this comment.
this is very easy to misuse,
what the server key should be? docs should mention it
if the server_key is wrong/has a typo, which function fails?
There was a problem hiding this comment.
I will add the validation for the variables. I left the naming the same as the fields in the structure, but yes, I will update the docs too.
| Method::Amp2Descriptor => { | ||
| let r: request::Amp2Descriptor = serde_json::from_value(params)?; | ||
| let mut s = state.lock()?; | ||
| if !matches!(s.config.network, lwk_wollet::ElementsNetwork::LiquidTestnet) { |
There was a problem hiding this comment.
why are you removing this?
do you need to enable regtest? if this is your intent, state it explicitly
There was a problem hiding this comment.
Yes, I'm using the regtest to test everything locally. And also, now, I'm declining only the mainnet network - https://github.com/kevandee/lwk/blob/feature/configurable-amp2-client/lwk_app/src/config.rs#L150-L153
There was a problem hiding this comment.
And yes, I'm using the CLI and app for manual tests, that's the fastest way for me to debug the proxy
There was a problem hiding this comment.
if you're adding regtest support, commit message should mention it.
| config.server_url = url; | ||
| } else if let Network::Regtest = args.network { | ||
| anyhow::bail!("on regtest you have to specify --electrum-url"); | ||
| anyhow::bail!("on regtest you have to specify --server-url"); |
There was a problem hiding this comment.
when you do unrelated fixes, do them in a separate commit
| } else if let Network::Regtest = args.network { | ||
| anyhow::bail!("on regtest you have to specify --amp2-keyorigin-xpub"); | ||
| } | ||
|
|
There was a problem hiding this comment.
when you extend lwk_cli functionalities, you should also extend lwk_cli tests to cover what you have added
|
if we want to allow callers to set amp2 url and xpub in app/cli, then you need to do the changes also on bindings and wasm |
| pub timeout: Duration, | ||
| pub scanning_interval: Duration, | ||
|
|
||
| pub amp2_url: Option<String>, |
There was a problem hiding this comment.
these should be string for consistency with the other fields (none of them is option)
pub amp2_url: String,
pub amp2_keyorigin_xpub: String,
| let amp_keyorigin_xpub = self.amp2_keyorigin_xpub | ||
| .clone() | ||
| .ok_or_else(|| Error::Generic( | ||
| "AMP2 methods are not available. AMP2 connection is not configured properly: missing amp_keyorigin_xpub".into() |
There was a problem hiding this comment.
error messages should suggest how to fix the issue, here you should speficy which flag should be passed
| if let Some(amp2_url) = amp2_url { | ||
| config.amp2_url = Some(amp2_url); | ||
| } else if let Network::Regtest = args.network { | ||
| anyhow::bail!("on regtest you have to specify --amp2-url"); |
There was a problem hiding this comment.
If I dont plan to do any amp2 calls, I should not be forced to specify this flag/option/param
2 participants
No description provided.