-
Notifications
You must be signed in to change notification settings - Fork 59
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
Track deals funding for deals that are being negotiated #336
Conversation
54ce6f1
to
6451b34
Compare
Codecov Report
@@ Coverage Diff @@
## master #336 +/- ##
==========================================
- Coverage 62.66% 62.45% -0.20%
==========================================
Files 40 41 +1
Lines 2651 2719 +68
==========================================
+ Hits 1661 1698 +37
- Misses 859 880 +21
- Partials 131 141 +10
Continue to review full report at Codecov.
|
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.
Generally LGTM but we need to release funds for all failures, not just immediately if EnsureFunds fails. See strategy outlined in comments
|
||
if err != nil { | ||
_, err2 := environment.DealFunds().Release(deal.Proposal.ClientBalanceRequirement()) |
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.
This needs to happen in the global fail state handler (FailDeal) - any error that happens between when EnsureFunds is called and the deal is published need to release the funds
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.
to know whether to release in FailDeal may mean recording a local value of funds reserved on the deal struct -- starts at 0 when deal is created, becomes ClientBalanceRequirement here, goes back to 0 after ValidateDealPublished, and then in the FailDeal handler, if reserved > 0, release those funds
|
||
if err != nil { | ||
_, err2 := environment.DealFunds().Release(deal.Proposal.ProviderCollateral) |
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.
again this needs to happen in the global fail state handler (FailDeal
) -- same comment as client
6451b34
to
1fe5e88
Compare
Release funds any time a deal fails, not just due to ensure funds error
Summary
Add a new interface,
DealFunds
, and use it to track outstanding funds needed for storage deals that are in progress.Resolves #331