feat: client flag to not announce deals #1051
Conversation
There was a problem hiding this comment.
Looks great so far 👍
As part of this PR, let's also add the fast retrieval and announce to IPNI fields on the deal detail page. This will have the side effect of making it easier to debug what happens when an old client (without knowledge of these flags) makes a request to a new version of boostd. Let me know if you need help with that.
Let's also add a test to provider_test.go with two subtests:
- Executes a deal with the AnnounceToIPNI flag set to false
- Executes a deal with the AnnounceToIPNI flag set to true
The code for the test itself should be quite short. It needs to
- set up the harness
- start
- execute
- wait for the deal to reach the IndexedAndAnnounced stage
- verify whether the deal was announced
However it may take some digging to understand how to set the parameters up and how to mock out the deal announcement code. Again let me know if you need help with that.
|
@LexLuthr I fixed the tests, so the only remaining part is the CLI changes. Please let me know when you've tested out the CLI changes and the UI changes. Note that I also removed one of the existing provider tests because it's over-complex and flaky. |
|
gql
|
cmd/boost/deal_cmd.go
Outdated
| Name: "keep-unsealed-copy", | ||
| Usage: "indicates that an unsealed copy of the sector should be available for fast retrieval", | ||
| Value: true, | ||
| }, | ||
| &cli.StringFlag{ | ||
| Name: "wallet", | ||
| Usage: "wallet address to be used to initiate the deal", | ||
| }, | ||
| &cli.BoolFlag{ | ||
| Name: "announce-to-ipni", | ||
| Usage: "indicates that deal index should be announced to the IPNI(Network Indexer)", | ||
| Value: true, | ||
| }, |
There was a problem hiding this comment.
These are new, never before existing CLI options: give them the same treatment as the protocol - explicitly asking for the less desirable thing
--remove-unsealed-copy
--skip-ipni-announce
There was a problem hiding this comment.
On the CLI we can set a default value more easily, so I would favour using a clearer name. To me boolean names are clearer when they're positive like "--announce-to-ipni" rather than negative "--skip-ipni-announce".
There was a problem hiding this comment.
Thanks Dirk and Riba! While the negative names (remove-unsealed-copy and skip-ipni-announce) are definitely annoying, I think we do want to keep the defaults of these booleans to false to align with general developer experience. In the case where an SP skips setting these flags all together, having the flags as false would be aligned with what they would expect.
Unfortunately... if we want the defaults to be false, then the names will be negative... :(
There was a problem hiding this comment.
When a user doesn't supply these flags on the command line, they will default to the value we have chosen for them.
In this case we have coded the parameter name to be --announce-to-ipni and the default value to be true.
Some example commands and the result:
| Flag value | Example Command | Whether index will be announced |
|---|---|---|
true |
boost deal --announce-to-ipni=true |
Yes |
false |
boost deal --announce-to-ipni=false |
No |
| Not set | boost deal |
Yes |
Note that the Command Line Interface is distinct from the Propose Storage Deal Protocol. Most SPs probably won't need to know of the protocol, they will just interact with the CLI tool.
There was a problem hiding this comment.
SP's doing deals with CLI are probably only testing and preparing to use the actual protocol though
There was a problem hiding this comment.
We talked sync and decided we will use the "negative" naming:
--remove-unsealed-copy (default false)
--skip-ipni-announce (default false)
| @@ -464,7 +465,8 @@ Response: | |||
| "Err": "string value", | |||
| "Retry": "auto", | |||
| "NBytesReceived": 9, | |||
| "FastRetrieval": true | |||
| "FastRetrieval": true, | |||
| "AnnounceToIPNI": true | |||
There was a problem hiding this comment.
These are auto-generated by the docs tool
There was a problem hiding this comment.
We should update the FastRetrieval naming to be aligned with the KeepUnsealedCopy name we are using. @dirkmc - if there's somewhere we are still using FastRetrieval, let's remove it to reduce confusion...
There was a problem hiding this comment.
Ok we can do that - note though that it will be a breaking change, so we would need to change the Propose Storage Deal Protocol major version (rather than just the minor version). This will require a little more work on our end.
There was a problem hiding this comment.
Sorry my mistake - these variable names are API methods that are called by the boost client.
I don't believe anyone outside our team is using these API methods so it should be fine to change them. But the naming is unrelated to this PR so I'd suggest we address it in a follow-up PR so that we can merge this one.
There was a problem hiding this comment.
that works for me, thank you!
| AnnounceToIPNI: Boolean! | ||
| KeepUnsealedCopy: Boolean! |
There was a problem hiding this comment.
Shouldn't the API match the protocol?
There was a problem hiding this comment.
This is just for display on the front end, and again I think it's clearer to use a positive name for booleans
* feat: client flag to not announce deals (#1051) * client flag to announce deals * fix itest * apply suggestions * test: fix announce tests * refactor: remove dead code * fix: TestCancelTransferForTransferredDealFails * fix var names * fix typo * refactor test * fix double negative * change flag names * change defaults value to false Co-authored-by: Dirk McCormick <dirkmdev@gmail.com> * fix: vacuum db after log cleanup (#1059) * vacuum db * fix syntax * vacuum at startup * log error * fix: run deal filter for offline deals (#1067) * run deal filter for offline deals * move filters to common func * fix: deal status on transfer complete (#1066) * fix: sort sealing worker list by date (#1074) Co-authored-by: LexLuthr <88259624+LexLuthr@users.noreply.github.com>
No description provided.