Skip to content
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

Refactor FundManager #4736

Merged
merged 6 commits into from
Nov 11, 2020
Merged

Refactor FundManager #4736

merged 6 commits into from
Nov 11, 2020

Conversation

dirkmc
Copy link
Contributor

@dirkmc dirkmc commented Nov 5, 2020

Currently in markets we have

  • a mechanism to reserve and release funds needed for a particular deal (no on-chain messages)
  • a mechanism to withdraw funds that must respect the reserved amount (calls withdraw on market actor)
  • a mechanism to ensure reserved funds are available when starting a deal (calls add funds on market actor)

This PR unifies the logic for these mechanisms into a single interface:

// Reserve adds amt to `reserved`. If there are not enough available funds for
// the address, submits a message on chain to top up available funds.
Reserve(ctx context.Context, wallet, addr address.Address, amt abi.TokenAmount) (cid.Cid, error)

// Subtract from `reserved`.
Release(addr address.Address, amt abi.TokenAmount)

// Withdraw unreserved funds. Only succeeds if there are enough unreserved
// funds for the address.
Withdraw(ctx context.Context, wallet, addr address.Address, amt abi.TokenAmount) (cid.Cid, error)

TODO:

Copy link
Contributor

@hannahhoward hannahhoward left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Early comments:

Overall, this is great. The code for the loop to handle reserves/releases/withdrawals is really good.

I have two changes, one fairly simple and the other... unfortunately not.

The first is your WithdrawFunds won't work as written. WithdrawBalance is a seperate method in the market actor.

The second is, we have to account for multiple wallets, and I'm not sure the best way to do this. Right now, at least on the client side, we assume that any wallet the client owns can initiate the deal. It's baked into the call to StorageClient.ProposeStorageDeal && all the way up to the Lotus full node API for ClientStartDeal. So, assuming a universal source wallet probably doesn't work. On the provider side, this is actually true -- the miner owner/worker address is always the source. However, there's no getting around it on the client. I'm sorry if my method signatures I provided obscured that fact.

I don't think the change is particularly hard, except your current code combines reserve requests & withdraw requests on the assumption of the same wallet. I believe the only way to ultimately address this is to add a source wallet parameter to Reserve and Withdraw, and to only combine messages for the same wallet address at the same time (deferring the messages for other wallets for the next message submission for the given destination address)

chain/market/fundmanager.go Outdated Show resolved Hide resolved
chain/market/fundmanager.go Outdated Show resolved Hide resolved
chain/market/fundmanager.go Outdated Show resolved Hide resolved
@arajasek arajasek added this to the 🖇Lotus Stabilization milestone Nov 6, 2020
@arajasek
Copy link
Contributor

arajasek commented Nov 6, 2020

Fixes #2685

Copy link
Contributor

@hannahhoward hannahhoward left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, pending integration. Remaining suggestions are non-blocking, up to you.

chain/market/fundmanager.go Outdated Show resolved Hide resolved
chain/market/fundmanager.go Show resolved Hide resolved
@dirkmc dirkmc mentioned this pull request Nov 10, 2020
2 tasks
@hannahhoward hannahhoward marked this pull request as ready for review November 10, 2020 23:47
@hannahhoward
Copy link
Contributor

I am marking this ready for review because I believe it can be merged without having any significant effect -- while there is new code, it is not actually active in Lotus yet as the integration aspects of this PR are not complete, and covered seperately in #4787 & filecoin-project/go-fil-markets#445

@hannahhoward
Copy link
Contributor

IOW, there is no harm in code reviewing this code, merging to master, in order to limit the challenge of reviewing #4787

Copy link
Contributor

@arajasek arajasek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@arajasek arajasek merged commit e53b0ef into master Nov 11, 2020
@arajasek arajasek deleted the refactor/fund-mgr branch November 11, 2020 04:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants