refactor(marketplace)!: new interface part 3 #72
Conversation
These are all automatically picked up by testNode
585e97f to
a3036db
Compare
a3036db to
c71adee
Compare
benbierens
left a comment
There was a problem hiding this comment.
I like where this is going.
| method unsubscribe*(subscription: OnChainMarketSubscription) {.async: (raises: []).} = | ||
| try: | ||
| await noCancel subscription.eventSubscription.unsubscribe() | ||
| except ProviderError: |
There was a problem hiding this comment.
Just for my info: when does ProviderError happen?
There was a problem hiding this comment.
The base method Subscription.unsubscribe() lists it as one of the errors that may be raised. JsonRpcProvider uses local subscriptions, which never raise an exception when unsubscribing, but we're using the generic Provider here.
| await clock.start() | ||
| success clock | ||
| except CancelledError as error: | ||
| raise error |
There was a problem hiding this comment.
If you omit these lines "except CancelledError" ... "raise error", wouldn't it result in the same thing?
There was a problem hiding this comment.
Unfortunately not. CancelledError is a subtype of CatchableError, so it would be picked up by except CatchableError.
| import ../utils/json | ||
| import ../manifest | ||
| import ../units | ||
| import ../clock |
There was a problem hiding this comment.
If this import wasn't required but now it is, I don't see why. :o
There was a problem hiding this comment.
Yes, we have too much export <module> code everywhere, so there are lots of surpising things in scope. And they sometimes surprisingly go out of scope when you remove something unrelated. This particular instance could very well be caused by the removal of contract interactions.
| validator = some ValidatorInteractions.new(clock, validation) | ||
|
|
||
| s.archivistNode.contracts = (client, host, validator) | ||
| s.archivistNode.marketplace = marketplace |
There was a problem hiding this comment.
The changes in this file make me so happy...
| check expectedData.len == data.len | ||
| check expectedData == data | ||
|
|
||
| test "slot repair calls onBatch callback and sets expiry": |
There was a problem hiding this comment.
I know this test isn't exactly a pearl of coding, but...
it was covering a testnet-discovered issue in which blocks resulting from repair had default-expiry instead of contract-duration-expiry.
I can disable node.nim:714 if err =? (await updateExpiry(@[blk])).errorOption: and I don't think any tests will catch this. Now I am entirely in favor of destroying this code. And the whole expiry system is being redesigned. But is there a way to cover this behavior? Or is it not worth the effort at this time?
There was a problem hiding this comment.
Yes, I agree. I intend to bring these tests back in #73, because then there is a nice interface to test against.
| let request = await testbed.request.nodes(3).submit(node) | ||
| let size = request.dataset.data.len | ||
| # start request | ||
| # 9 slots makes the odds that all slots are filled by one node negligable |
There was a problem hiding this comment.
This smells like it could go wrong once in a great while.
What I did is: calculate the slot size and set each provider availability to be slot size + 10%
Then have nodes = 3 and 4 providers.
You can use the slotFilled events to find any one provider and stop it and you're also guaranteed that there's capacity for repair.
There was a problem hiding this comment.
Yes, this test did something similar before. But now we can no longer set the provider availability size, because with the changes in this PR we can only set the repostore quota. And the node will simply try to sell the entire quota. I tried restricting the quota for the nodes in this test, but that didn't work because each node would try to fill 3 slots simultaneously (3 slotqueue workers). So that's why I chose these request parameters. I came to a rough estimate that this test could go wrong once every 250 000 runs, which I deemed acceptable.
MarketplaceNodeas the main interface for sales, purchasing and validation.