- 
                Notifications
    You must be signed in to change notification settings 
- Fork 10
Use Vault in Marketplace #223
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
b9689f1    to
    0f0f830      
    Compare
  
    32d0dbd    to
    095e90f      
    Compare
  
    53679b4    to
    cfd30bf      
    Compare
  
    6a7d559    to
    957f6ec      
    Compare
  
    cfd30bf    to
    8a11e87      
    Compare
  
    15d50c7    to
    55fb29f      
    Compare
  
    55fb29f    to
    0bdefa7      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, big effort! 👏
Generally LGTM, but there are a few critical points that need clarification or addressing, so I will go for the "Request changes" here.
I have gone over the contract changes, still need to see the testing of this, but that will be in round two 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Generally LGTM! Just few comment here and there, but nothing blocking ;-)
Thanks again for such a big chunk of work 👏
| await waitUntilStarted(marketplace, request, proof, token) | ||
| await waitUntilFinished(marketplace, requestId(request)) | ||
| await marketplace.freeSlot(slotId(slot)) | ||
| expect(await marketplace.currentCollateral(slotId(slot))).to.equal(0) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add balances checks that verify what happens to the collateral? Eq. burned vs. transfered back? Also for the tests bellow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm missing the point, but the balances are checked in the tests right above these tests ("pays out finished request base on time hosted", "pays the host when contract was cancelled" and "pays the host when contract fails and then finishes")
Vault stores balances as uint128
replaced by formal verification with certora
- removes RequestCancelled event, which was not great anyway because it is not emitted at the moment that the request is cancelled
- move all collateral calculatons to separate library
so that they can no longer be transfered within the vault
This state is no longer necessary, vault ensures that payouts happen only once. Hosts could bypass this state anyway by withdrawing from the vault directly.
- changes to marketplace constructor - we no longer have _marketplaceTotals - timestamps have their own type now - freeSlot no longer takes payout addresses - slot state 'Paid' no longer exists - freeSlot can be invoked more than once now - a failed request no longer ends immediately
No need to test the token itself
It is no longer a discount on the collateral, to simplify reasoning about collateral in the sales module of the codex node
So that a storage provider can know how much collateral is returned when it calls freeSlot()
Because a client might still need to call withdraw()
reason: freeSlot() will call forciblyFreeSlot when contract is not finished, so we can use that instead
reason: hosts are paid for the time that they hosted the slot up until the time that the request failed Co-Authored-By: Adam Uhlíř <[email protected]>
Co-Authored-By: Adam Uhlíř <[email protected]>
reason: the function only depends on the request, and it is similar to the pricePerSlotPerSecond function Co-Authored-By: Adam Uhlíř <[email protected]>
Co-Authored-By: Adam Uhlíř <[email protected]>
Co-Authored-By: Adam Uhlíř <[email protected]>
Co-Authored-By: Adam Uhlíř <[email protected]>
Co-Authored-By: Adam Uhlíř <[email protected]>
1ca2091    to
    4410941      
    Compare
  
    Co-Authored-By: Adam Uhlíř <[email protected]>
Co-Authored-By: Adam Uhlíř <[email protected]>
Uses the vault for all token transfers in the marketplace. The marketplace now no longer keeps any funds itself.
This has impact on the following as well:
Timestamp,DurationandTokensPerSecondon the API