Add optional boundaries to getregistrationreceipt

[ShubhamBhut]

fixes #97

@mariocynicys @sr-gi can you pls have a look at this ? I am actually pretty new to rust.

[sr-gi]

First pass. Check comments inline.

Thanks for taking care of this, I love the proactivity.

Notice that the command does not work as intended at the moment if you pass more than one parameter to it. e.g:

lightning-cli getregistrationreceipt 03488e12bc580caaaa4f575bde2b4754746382fc5e8b5e3d03a632b0f15c5175cc 0 1
"Unexpected json format. Expected a single parameter. Received: 3"

This is due to the TowerId parsing I mentioned in one of the comments.

Notice, also, how you have only patched the test to cover the old case, but there are no tests for the new cases (hence why you didn't catch the errors on the updated function). This requires tests for both the old and the new cases, so we can make sure that both the latest receipt can be pulled if no boundary is specified, but also that every receipt within a boundary is returned.

Also, for the RPC specific changes, I'll suggest you manually try all the possible inputs (there are only two options, either start and end blocks are specified or not).

[sr-gi]

None of this is required. There is no such thing as a default subscription start or expiry. They depend on the block height at which the user requested the subscription.

[sr-gi]

Notice that you've changed the behavior of this method.

If the bounds are not passed, the method should work exactly as it was before. Otherwise, it should have this new functionality.

Also, why are you limiting the result? This can potentially return multiple items.

[ShubhamBhut]

Got it, I will keep exactly old method in case of no optional parameters.
Actually I thought since old function was returning only latest receipt, new one should do same. I will remove the limit.

[sr-gi]

There's no need for two methods tho, just build the SQL query based on what the input params are.

[sr-gi]

Check the params macro, I don't think we need to make it this complicated

[sr-gi]

This is not right. Given we are now potentially passing multiple params to the function, we cannot crate the TowerId as before, given v now can contain more data.

What is passed the the function is a json of the input params. Check in convert.rs how other commands handle this (you'll need to create a GetRegistrationReceiptParams struct and implement the parsing)

[ShubhamBhut]

Oh my bad, will fix this.

[ShubhamBhut]

I have implemented RegistrationReceiptParams struct, but it seems when optional arguments are passed, the query fails, I haven't been able to figure out exact issue in code, code seems fine to me. Can you pls have a look

[sr-gi]

Check comments inline.

You have still not provided tests where the boundaries are checked, only cases where both are None.

Regarding why this is not properly returning when you call it with the boundaries, the reason is that you are passing start and expiry as be_bytes, while they are defined in the database as numbers. You need to pas them as such.

[sr-gi]

This line break is not needed

[sr-gi]

Same here, there's not actual change, this does not need reformating

[sr-gi]

Both start and expiry need to be passed for the query to be valid. This is checking if they are independently passed (one OR the other, but not one AND the other).

You can either check this here and return an error, or in the main function and not call this method if the constraint is not met.

[sr-gi]

As mentioned in the database method, this should only accept ether none or both sides of the boundary. I think this should be the right place to make that check, and fail to parse if both are not provided. That is, the parsing accepts either 1, or 3 params.

[ShubhamBhut]

I have made all the suggested changed, new function seems to be working perfectly in my local device.
Though I am getting output mismatch error for subscription_expiry in test_load_registration_receipt even though the function queries for exact same params used to store receipt in db.
error log:

thread 'dbm::tests::test_load_registration_receipt' panicked at 'assertion failed: `(left == right)`
  left: `RegistrationReceipt { user_id: UserId(PublicKey(7a7fd8cca7fd5d5e4584f7c6e1453ab67a7b834601c0bb04d9442dba9edee9f860947791f39d7033
d6a5de0e1b221472c60f82abc6585b959ddb399e17383844)), available_slots: 1414565319, subscription_start: 741713076, subscription_expiry: 7417
13496, signature: Some("dhrb3euw99g7gfrhf68ifwsnknro5ksbhs5zh4tfyapqbx5yb3d7rj4hih14fb8in6mmmgihng876pq1dy17t9jfcc1txaq7kycufdet") }`,
 right: `RegistrationReceipt { user_id: UserId(PublicKey(7a7fd8cca7fd5d5e4584f7c6e1453ab67a7b834601c0bb04d9442dba9edee9f860947791f39d7033
d6a5de0e1b221472c60f82abc6585b959ddb399e17383844)), available_slots: 1414565521, subscription_start: 741713076, subscription_expiry: 7417
13626, signature: Some("ryxxzerta61taatusksbcgpfkpdsaa7xb8ybsw9pwuath5nseqigndqp4ikhi6gemuaaaiza9ar7ufz8oi9p3k861xb1g69a76ndy6ky") }`', w
atchtower-plugin/src/dbm.rs:752:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

[sr-gi]

This looks better now. There are still some things to have in mind though:

Notice that, given now we are working with a boundary, it could be the case that multiple registration receipts are a match for a certain boundary. That should be taken into account. This means that the dbm method will need to return a vector instead of a single item.

Also, the naming of the methods may need to be updated to make sense.

Regarding tests, you have updated them to cover the case where a boundary is passed, but now the case where it is not passed is not covered anymore. Think about what are all possible combinations of inputs and do test them all. Also test when multiple receipts are returned by the method

[sr-gi]

This is not needed.

Please do check unnecessary formatting changes before pushing.

[sr-gi]

You can merge all this into a single if let statement.

At this point you have already made sure that the provided number of params is either one or three, so there is no way the user is providing the subscription_start but not subscription_end.

[sr-gi]

Also, we are referencing subscription_start and subscription_expiry in snake case, but you are logging them as Subscription-{start, end}

[sr-gi]

nit: add a line break here

[sr-gi]

Optional parameters are usually expressed in square brackets, so this should read something like:

Unexpected request format. Expected: tower_id [subscription_start] [subscription_expire]. Received: '{value}'

[sr-gi]

If you want to emphasize the boundary not being fully passed (given this may be a common error), you can have a special case here for two params before checking one or three were you error that if a boundary is set, both ends of the boundary need to be specified. e.g:

if parame_count == 2 {...}
else if param_count != 1 && param_count != 3 {...}
else {...}

[sr-gi]

This feels pretty hacky, especially when there is no boundary, well be passing (AND NULL is NULL) AND (NULL is NULL) which will be valid, but feels pretty unnecessary.

It'll be best to find a way to define the query and params list depending on whether subscription_start and subscription_expiry were passed

[sr-gi]

Notice this is not the right error for every case now. It used to be the case because if no receipt could be pulled from the database it meant the tower was unknown. Now, if params have been passed, it could be the case that you could not find a receipt for the specified boundary. This needs to be covered. I'd suggest you check whether the tower can be found in memory if the query errors, and the return the proper error depending on that outcome.

[ShubhamBhut]

Hey @sr-gi,
Sincere apologies for the formatting mistakes. I believe this commit should solve everything; except I couldn't think of any new & better name for the updated methods.

[sr-gi]

I think this may be working now.

A few extra comments. I'll need to give a final pass to this once everything is covered.

All commits need to be squashed.

[sr-gi]

A space is missing after {

[sr-gi]

Not sure why rust fmt is not properly formatting this, but here you have a proper formatted version:

diff --git a/watchtower-plugin/src/convert.rs b/watchtower-plugin/src/convert.rs
index d49889c..b9475de 100644
--- a/watchtower-plugin/src/convert.rs
+++ b/watchtower-plugin/src/convert.rs
@@ -289,20 +289,22 @@ impl TryFrom<serde_json::Value> for GetRegistrationReceiptParams {
         match value {
             serde_json::Value::Array(a) => {
                 let param_count = a.len();
-                if param_count == 2{
-                    Err(GetRegistrationReceiptError::InvalidFormat(("Both ends of boundary (subscription_start and subscription_expiry) are required.").to_string()))
+                if param_count == 2 {
+                    Err(GetRegistrationReceiptError::InvalidFormat(
+                        "Both ends of boundary (subscription_start and subscription_expiry) are required".to_string()
+                    ))
                 } else if param_count != 1 && param_count != 3 {
                     Err(GetRegistrationReceiptError::InvalidFormat(format!(
                         "Unexpected request format. The request needs 1 or 3 parameter. Received: {param_count}"
                     )))
-                } else{
+                } else {
                     let tower_id = if let Some(s) = a.get(0).unwrap().as_str() {
                         TowerId::from_str(s).map_err(|_| {
-                        GetRegistrationReceiptError::InvalidId("Invalid tower id".to_owned())
+                            GetRegistrationReceiptError::InvalidId("Invalid tower id".to_owned())
                         })
                     } else {
                         Err(GetRegistrationReceiptError::InvalidId(
-                        "tower_id must be a hex encoded string".to_owned(),
+                            "tower_id must be a hex encoded string".to_owned(),
                         ))
                     }?;

@@ -311,22 +313,24 @@ impl TryFrom<serde_json::Value> for GetRegistrationReceiptParams {
                             (Some(start as u32), Some(expire as u32))
                         } else {
                             return Err(GetRegistrationReceiptError::InvalidFormat(
-                                    "Subscription_start must be a positive integer and subscription_expire must be a positive integer greater than subscription_start".to_owned(),
-                                    ));
+                                "subscription_start must be a positive integer and subscription_expire must be a positive integer greater than subscription_start".to_owned(),
+                            ));
                         }
                     } else if a.get(1).is_some() || a.get(2).is_some() {
                         return Err(GetRegistrationReceiptError::InvalidFormat(
-                                "Subscription_start and subscription_expiry must be provided together as positive integers".to_owned(),
-                                ));
+                             "subscription_start and subscription_expiry must be provided together as positive integers".to_owned(),
+                        ));
                     } else {
                         (None, None)
                     };

-                    Ok(Self {
-                    tower_id,
-                    subscription_start,
-                    subscription_expiry,
-                    })
+                    Ok(
+                        Self {
+                            tower_id,
+                            subscription_start,
+                            subscription_expiry,
+                        }
+                    )
                 }
             },
             serde_json::Value::Object(mut m) => {

[ShubhamBhut]

Thanks for sharing the formatted code. Both rustfmt and cargo fmt are not formatting this code. Did it work for you or you had to format manually ?

[sr-gi]

Unfortunately I had to do it manually, I don't know why rust fmt seems not to properly handle big files with multiple levels of identation :(

[sr-gi]

I don't think this branch is really necessary, is it?

We've already made sure that both the start and the end of the boundary have been provided, or none of them have. At this point is either the boundary is correct (start>=0 and end>start) or it is not, so it cannot be the case that either of them but not both are Some(x)

[ShubhamBhut]

Actually the method was also allowing text(alphabetic letters) as subscription_start and subscription_expiry. I added this branch to handle this case. Should I remove this ?

[sr-gi]

Oh, right, that's because elements are passed as json Values and they can be anything really. I think this should be re-arranged then for clarity, since I didn't really get what was the point of having that else.

Can you make it so you capture the values in your if let as you are atm, but instead of convert them straight away you leave them as Values, and then you try to convert them to i64 or fail with a similar error to this one inside that same context?

e.g.:

let (subscription_start, subscription_expiry) = if let (Some(start), Some(expire)) = (a.get(1)), a.get(2)) {
    /// Try to convert them to i64, fail if they are not convertable
}

[sr-gi]

I think we should keep this an Option, so its either None or Some(Vec<...>)

[mariocynicys]

The thing is, after reworking this function to return a vector, it should never return None. It will return an empty vector if 1- the tower wasn't found & 2- the given range doesn't intersect with any subscription.

[sr-gi]

You don't need to collect this

[ShubhamBhut]

hey @sr-gi can you pls eloberate a little ? I am not seeing any collect here and I believe that below(at line 248) collect is required. Also I have changed that code to the code recommended by Omer now.

[sr-gi]

By collecting I mean storing it in a temp variable. You're doing:

let receipts = stmt.query_map...
receipts

But you can simply return the statement call:

stmt.query_map...

[sr-gi]

I think we can simplify this to Result<_, _>. I'm not really sure why the annotation is needed tbh.

Any ideas @mariocynicys?

[mariocynicys]

I would have written it like this:

let receipts = stmt
    .query_map(params.as_slice(), |row| {
        let slots: u32 = row.get(0)?;
        let start: u32 = row.get(1)?;
        let expiry: u32 = row.get(2)?;
        let signature: String = row.get(3)?;

        Ok(RegistrationReceipt::with_signature(
            user_id, slots, start, expiry, signature,
        ))
    })
    .unwrap() // MappedRows<|&Row| -> Result<RegistrationReceipt, E>>
    .map(|r| r.unwrap())
    .collect();

It's pretty interesting though how the type after the .unwrap() (MappedRows<|&Row| -> Result<RegistrationReceipt, E>> which is equivalent to a Vec<Result<RegistrationReceipt, E>> when collected) could be collected into Result<Vec<RegistrationReceipt>, E>.

[mariocynicys]

I think we can simplify this to Result<_, _>. I'm not really sure why the annotation is needed tbh.

I missed the point here 😂, yeah Result<_, _> is valid.

I would say .map(|res| res.unwrap()).collect(); feels more intuitive/less-hacky though.

[sr-gi]

There's something I just realized while manually testing this.

IIRC if two subscriptions overlapped, the user was only supposed to be storing the latest receipt. However, I just tested this and two receipts were returned where receipt_a ranged Ta-Tb, and receipt_b ranged Ta-Tc with c>b.

Does this ring a bell @mariocynicys?

No worries @ShubhamBhut, this is not due to your PR, but we may have forgotten to remove some data in an older PR.

[mariocynicys]

IIRC if two subscriptions overlapped, the user was only supposed to be storing the latest receipt. However, I just tested this and two receipts were returned where receipt_a ranged Ta-Tb, and receipt_b ranged Ta-Tc with c>b.

Revising this, we made the merger on the tower side and not the client side.
The client side though gets it's subscription summary in memory updated to the merger of the two overlapping receipts, but all the receipts are stored in the DB nonetheless.
We should delete those receipts since the last one already covers all of the past overlapping ones, so to get a cleaner output from getregistrationreceipt.

[sr-gi]

IIRC if two subscriptions overlapped, the user was only supposed to be storing the latest receipt. However, I just tested this and two receipts were returned where receipt_a ranged Ta-Tb, and receipt_b ranged Ta-Tc with c>b.

Revising this, we made the merger on the tower side and not the client side. The client side though gets it's subscription summary in memory updated to the merger of the two overlapping receipts, but all the receipts are stored in the DB nonetheless. We should delete those receipts since the last one already covers all of the past overlapping ones, so to get a cleaner output from getregistrationreceipt.

Referenced this here #208

[ShubhamBhut]

I am having some issues with squashing, I will submit squashed PR after resolving them. I am submitting code for review in case my squashing issues remain longer.

[sr-gi]

Some small changes. Looks good otherwise.

Remember to squash the commits.

[sr-gi]

You can simplify this:

Suggested change

	stmt.query_map(params.as_slice(), |row| {
	let slots: u32 = row.get(0)?;
	let start: u32 = row.get(1)?;
	let expiry: u32 = row.get(2)?;
	let signature: String = row.get(3)?;
	Ok(RegistrationReceipt::with_signature(
	user_id, slots, start, expiry, signature,
	))
	})
	.unwrap()
	.map(|r| r.unwrap())
	.collect(),
	)
	stmt.query_map(params.as_slice(), |row| {
	let slots: u32 = row.get(0)?;
	let start: u32 = row.get(1)?;
	let expiry: u32 = row.get(2)?;
	let signature: String = row.get(3)?;
	Ok(RegistrationReceipt::with_signature(
	user_id, slots, start, expiry, signature,
	))
	})
	.unwrap()
	.map(|r| r.ok())
	.collect()

[sr-gi]

This is not true anymore

[sr-gi]

Remove extra space before "?"

[sr-gi]

Given this implies that the tower is found, an it cannot be the case that a tower is found and a registration receipt is not, it means it can only be hit if a boundary is passed and there is no receipt for that boundary. Therefore:

Suggested change

	Err(anyhow!("No registration receipt found for {tower_id}"))
	Err(anyhow!("No registration receipt found for {tower_id} on the given range"))

[sr-gi]

ACK fc794fb

[sr-gi]

@mariocynicys care to give this a look when you have some time?

[mariocynicys]

Thanks!

Some nits and this is nearly finished, but there are some stuff I didn't grasp.

[mariocynicys]

Insert a space after (a.get(1), a.get(2)) before {.

[sr-gi]

Has this been fixed?

[mariocynicys]

nit: can we name this variable expiry?

[mariocynicys]

If we want it a positive integer, shouldn't we use as_u64?

[mariocynicys]

Can't they be the same value? If, for instance, I know what specific block I registered in.

[mariocynicys]

missing a space here as well, before the {.

[mariocynicys]

please add a ; at the end of this line.

[mariocynicys]

and this one as well.

[mariocynicys]

please fix the spacing around the >= & <= (keep one space on each side).

[mariocynicys]

I don't really grasp this part. why is there an .unwrap()?!
response is an optional vector, so Option::None & an empty vector signal different meanings? right?

I guess Option::None is when the tower isn't found and an empty vector is when the registration range doesn't contain any receipts?
If so, then the .clone().unwrap() part will panic with no useful information to the user.

Otherwise, we don't need the None variant anymore.

[sr-gi]

Yeah, that is actually what i think I was suggesting for, however, checking DBM::load_registration_receipt I don't think we ever produce a None return.

[sr-gi]

I'd be good to test this out by creating a test that queries a tower that is not part of the DB and see whether the method is returning an empty vec or None

[mariocynicys]

I can't really get my head around how collect() produces Option<Vec< here 😕,
but aside from that, I see that the failure that's being accounted for in map(|r| r.ok()) is for the de-serialization of the data from the DB (slots, start, ...) which we assume shouldn't fail anyways.

I think we can unwrap while de-serializing directly since this shouldn't actually fail.
and substitute map(|r| r.ok()) with map(|r| r.unwrap()). This will make this method return a Vec< instead.

[ShubhamBhut]

I think this was previous approach, but then sr-gi suggested to keep it Option<Vec<
ref

[sr-gi]

I think my point here was being able to distinguish between when no data was found because the tower was not even there, and when the range didn't produce any output. However, we are checking that afterward in main.rs by checking if the tower was part of the state. So this may indeed be simplified.

[mariocynicys]

I think my point here was being able to distinguish between when no data was found because the tower was not even there, and when the range didn't produce any output.

I would be for implementing this kind of logic, but the one implemented in here doesn't actually distinguish between the two cases. It always returns Some(a_possibly_zero_len_vec).

[sr-gi]

Yeah, that is my point, that the suggestion was for that, but we really don't do that here, but always return Some(...) and check afterward.

I'm happy with either one or the other.

[mariocynicys]

There is no distinction in usage between these two different error variants. I think we can combine them into one.
We might also be able to get rid of them and use an Err instead.

[ShubhamBhut]

Actually I looked how other methods had this implemented and followed them 😅. Which way do you think would be better, keeping as it is, merging into one error or removing this entirely ? I will make changes accordingly.

[mariocynicys]

Yeah I noticed other param parsers are doing the same. I think we can make all of them to use Err(String)||Err(&'static str).

Which way do you think would be better, keeping as it is, merging into one error or removing this entirely ? I will make changes accordingly.

Try to remove it entirely, then we can do the same for other param errors in a follow up if it made sense.

[ShubhamBhut]

Err seems to be working fine, just had to tweak the code where parsing was done. I think I would wait for decision on issue of Vec or Option<vec before final submission, rest changes are done.

[sr-gi]

The reason why this is here is to make it properly testable. If you look at the other params, there are test suits to make sure the proper error is returned.

I guess we should add them too for GetRegistrationReceipt now

[ShubhamBhut]

oh, got it. I will add tests for GetRegistrationReceipt params there. And in case of #199 (comment), I think it would be better as Some(Vec<>) just because its common way in rust-teos code.

[mariocynicys]

And in case of #199 (comment), I think it would be better as Some(Vec<>) just because its common way in rust-teos code.

This would only make sense if #199 (comment) is carried out. I am fine with both ways, but the None variant has to carry a meaning and not just be there so the interface look similar to other methods.

[sr-gi]

Agreed