Use `add_lease` instead of `renew_lease`

Fixes #220 (closed)

Maybe also #224 (closed)

Created by: codecov[bot]

Codecov Report

Merging #226 (be4d9cf9) into main (e4ba6c78) will increase coverage by 0.01%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #226      +/-   ##
==========================================
+ Coverage   96.97%   96.98%   +0.01%     
==========================================
  Files          49       49              
  Lines        3997     4011      +14     
  Branches      506      509       +3     
==========================================
+ Hits         3876     3890      +14     
  Misses         79       79              
  Partials       42       42              

Impacted Files	Coverage Δ
src/_zkapauthorizer/lease_maintenance.py	95.38% <100.00%> (+0.07%)
src/_zkapauthorizer/tests/strategies.py	96.27% <100.00%> (ø)
...rc/_zkapauthorizer/tests/test_lease_maintenance.py	98.85% <100.00%> (+0.08%)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update e4ba6c7...be4d9cf. Read the comment docs.

Created by: tomprince

Review: Approved

This looks good to me.

I noticed that you didn't do the second part of my suggestion from #220 (closed). Looking at the code again, I guess this is because we don't have enough information (i.e. the cancel secret) to do so. I wonder if it makes sense to instead remove the method from ZKAPAuthorizerStorageServer and have it raise NotImplemented in ZKAPAuthorizerStorageClient (I guess the first one would have compatibility implications for our unreleased software)?

This looks good to me.

I noticed that you didn't do the second part of my suggestion from #220 (closed). Looking at the code again, I guess this is because we don't have enough information (i.e. the cancel secret) to do so. I wonder if it makes sense to instead remove the method from ZKAPAuthorizerStorageServer and have it raise NotImplemented in ZKAPAuthorizerStorageClient (I guess the first one would have compatibility implications for our unreleased software)?

Indeed. Thank you for following up on this point. I think maybe at the time I just forgot about that idea. Thinking about it now, esp. in the light of the fact that (a) Itamar discovered that there aren't actually any callers of renew_lease in the Tahoe-LAFS code base (apart from tests) and (b) Tahoe-LAFS has now actually dropped renew_lease from the network protocol entirely (ie you're not even allowed to call it anymore)...

• Against a Tahoe-LAFS <1.16 storage server a custom client could call our renew_lease so if we provide it we still need to do ZKAP authorization there. But the standard client never does so we could also just delete our renew_lease implementation entirely.
• Against a Tahoe-LAFS >=1.16 storage server a custom client can no longer even get a renew_lease call deserialized (or whatever ... Foolscap will refuse to dispatch it) on the server. So for this case we could also just delete our renew_lease implementation entirely.
• No version of the Tahoe-LAFS client ever called renew_lease so the client-side version doesn't matter.

So ... I'll file another ticket for deleting all of our renew_lease code, I think, and apparently there aren't even any compatibility implications to worry about (though I guess I'll still worry about them and test them out somehow to be sure).