Implement listadresses #709

pythcoiner · 2023-09-09T18:16:45Z

address #681

todo:

implement tests
update docs

edit: i'm really new to rust, don't hesitate to kick my ass when i write stupid code

src/commands/mod.rs

tests/test_rpc.py

darosior

Approach ACK. Needs a bit more testing (and an entry in doc/API.md) but it's in good shape!

src/commands/mod.rs

tests/test_rpc.py

src/commands/mod.rs

darosior · 2023-09-12T09:19:29Z

Let me know when this is ready for review by moving this out of draft / WIP. No rush!

pythcoiner · 2023-09-12T18:32:32Z

Let me know when this is ready for review by moving this out of draft / WIP. No rush!

i think it is now ;)

doc/API.md

pythcoiner · 2023-09-13T10:23:16Z

thx @jp1ac4 for your review, comments have been addressed in 4e0bc0a

tests/test_rpc.py

jp1ac4 · 2023-09-13T14:25:34Z

src/jsonrpc/api.rs

@@ -258,6 +282,14 @@ pub fn handle_request(control: &DaemonControl, req: Request) -> Result<Response,
        }
        "getinfo" => serde_json::json!(&control.get_info()),
        "getnewaddress" => serde_json::json!(&control.get_new_address()),
+        "listaddresses" => {
+            let params = req.params.ok_or_else(|| {


As the parameters are optional, I'm wondering if you could keep params as Option<Params> and pass that to list_addresses.

I'm able to run listaddresses without any parameters so I guess req.params is Some also in that case and it works fine, but not sure if there might be a situation where it is None.

@darosior may be able to help.

you seems right, i wonder why Error::invalid_params is not triggered when call listaddresses w/o args? shouldn't it in that case?

That's most likely you tested with the CLI which always includes a params entry in the JSONRPC request. But that's not mandated by the JSONRPC2 specs.

I agree with @jp1ac4 this shouldn't error if params is None. You can take inspiration from how list_coins does this.

darosior · 2023-10-07T11:58:40Z

Could you rebase and squash your commits into a couple ones? I'll review then.

pythcoiner · 2023-10-07T14:28:49Z

I rebased and squash to 3 commits implement/test/fixes, feel free to tell me if you prefer I squash to a single one.

darosior · 2023-10-08T09:55:10Z

Having tests in a separate commit is fine. However having fixups in a separate commit than the one where issues were introduced creates needless churn and confusion.

That's fine for this time, but next time please squash the fixup commit(s) in the appropriate original commit(s).

darosior

Almost there, the main things are:

There is an off by one when count is not provided
Let's not crash on insane inputs
Let's not error if a params entry isn't given in the JSONRPC request, since all fields are optional

doc/API.md

darosior · 2023-10-08T10:02:33Z

src/commands/mod.rs

+                let child = bip32::ChildNumber::from_normal_idx(index)
+                    .expect("Failed to convert index to ChildNumber");


Could we error instead of crashing if a value greater than 2**31 - 1 is passed for count? It's an insane argument to give to the command, but it's nicer to never crash on insane inputs.

Let's add an InvalidAddressCount variant to the CommandError enum and use it here and above?

src/commands/mod.rs

darosior · 2023-10-08T10:16:57Z

src/jsonrpc/api.rs

@@ -258,6 +282,14 @@ pub fn handle_request(control: &DaemonControl, req: Request) -> Result<Response,
        }
        "getinfo" => serde_json::json!(&control.get_info()),
        "getnewaddress" => serde_json::json!(&control.get_new_address()),
+        "listaddresses" => {
+            let params = req.params.ok_or_else(|| {


That's most likely you tested with the CLI which always includes a params entry in the JSONRPC request. But that's not mandated by the JSONRPC2 specs.

I agree with @jp1ac4 this shouldn't error if params is None. You can take inspiration from how list_coins does this.

tests/test_rpc.py

pythcoiner · 2023-11-06T09:17:42Z

comment have been adressed

darosior · 2023-11-10T12:04:08Z

This comment wasn't addressed? #709 (comment)

pythcoiner · 2023-11-10T14:02:05Z

This comment wasn't addressed? #709 (comment)

right, it's done now

darosior · 2023-11-11T12:32:38Z

tests/test_rpc.py

@@ -37,6 +37,24 @@ def test_getaddress(lianad):
    assert res["address"] != lianad.rpc.getnewaddress()["address"]


+def test_listadresses(lianad):


nit:

Suggested change

def test_listadresses(lianad):

def test_listaddresses(lianad):

darosior · 2023-11-11T13:00:14Z

src/commands/mod.rs

+            Self::InvalidAddressCount => write!(f, "Invalid address count, should be under 2^31-1"),
+            Self::InvalidAddressIndex => write!(f, "Invalid address index, should be under 2^31-1"),


Would be better to introduce a single enum variant, a general "invalid derivation index" which could be reused in the future.

darosior · 2023-11-11T13:01:22Z

src/commands/mod.rs

+        if start_index > (2u32.pow(31) - 1) {
+            return Err(CommandError::InvalidAddressIndex);
+        }


(Here and below) Ok, this works. But it would be much clearer to use ChildNumber::from_normal_idx's error or at the very least add a comment.

darosior · 2023-11-11T13:02:27Z

src/commands/mod.rs

+            return Err(CommandError::InvalidAddressIndex);
+        }
+
+        let count = count.unwrap_or_else(|| receive_index.max(change_index) - start_index);


What if start_index is larger than the maximum of the receive and change indexes? We need to really be careful about underflows in this code.

darosior · 2023-11-11T13:03:47Z

src/commands/mod.rs

+        if count == 0 {
+            let out: Vec<AddressInfo> = Vec::new();
+            return Ok(ListAddressesResult::new(out));
+        }


It's a bit ugly to special case 0 here to avoid the checked_sub below to return None. Could get rid of the necessity for a checked_sub altogether by using an exclusive range ((x..y)) below instead of an inclusive one ((x..=y)).

darosior · 2023-11-11T13:04:11Z

src/commands/mod.rs

@@ -860,6 +927,24 @@ impl GetAddressResult {
    }
 }

+#[derive(Debug, Clone, Serialize)]


Would be nice to make it PartialEq, Eq too.

darosior · 2023-11-11T13:04:39Z

src/commands/mod.rs

+    #[test]
+    fn listaddresses() {


Would be good to check bounds here for hardened derivation index, overflows and underflows.

darosior · 2023-11-11T13:18:21Z

src/jsonrpc/api.rs

+        .as_ref()
+        .and_then(|p| p.get(0, "start_index"))
+        .and_then(|i| i.as_u64())
+        .and_then(|i| i.try_into().ok());


We shouldn't silently ignore invalid parameters to listaddresses.

darosior · 2023-11-11T13:33:07Z

I've addressed my comments here: https://github.com/darosior/liana/tree/get_all_addresses. Apparently i can't push them to your PR, so i'll open a follow-up..

darosior · 2023-11-11T13:34:15Z

See #808.

darosior · 2023-11-11T13:43:05Z

ACK 2660b77 -- my requests are addressed in followup #808.

0812a12 jsonrpc: don't ignore invalid params to listaddresses (Antoine Poinsot) d533820 commands: listaddresses cleanups (Antoine Poinsot) Pull request description: This addresses my latest review from #709. ACKs for top commit: jp1ac4: ACK 0812a12 Tree-SHA512: 6f708fd2f1aa2f229a5c78a35e363870ef390513cce10fc6a5938b49e6b7ee5be9205bc4566376750e4f7eeea404709ce6d8c7a29df15b9216b8dbcf4b4fed7e

pythcoiner commented Sep 9, 2023

View reviewed changes

src/commands/mod.rs Outdated Show resolved Hide resolved

pythcoiner commented Sep 9, 2023

View reviewed changes

src/commands/mod.rs Show resolved Hide resolved

pythcoiner mentioned this pull request Sep 10, 2023

Address list #681

Closed

pythcoiner commented Sep 10, 2023

View reviewed changes

tests/test_rpc.py Show resolved Hide resolved

darosior reviewed Sep 11, 2023

View reviewed changes

pythcoiner commented Sep 11, 2023

View reviewed changes

src/commands/mod.rs Outdated Show resolved Hide resolved

pythcoiner force-pushed the get_all_adresses branch 4 times, most recently from 08d7f7a to 646c525 Compare September 12, 2023 08:40

pythcoiner marked this pull request as ready for review September 12, 2023 18:31