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

Claiming expenditure payout #925

Merged
merged 18 commits into from
Aug 23, 2023

Conversation

jakubcolony
Copy link
Collaborator

@jakubcolony jakubcolony commented Aug 7, 2023

Description

This PR adds claiming expenditure payout functionality.

After finalization, you should see a Claim funds button appear that lets you claim all the payouts for which the claim delay has elapsed (see below for more info on how claim delay works in dev).

Testing

  1. Make sure you restart your entire dev environment as there was a network version upgrade.
  2. Create and advance expenditures through all the stages to finalization.
  3. Attempt to claim the funds. Note: It is expected that attempts to claim a locked token (e.g. from another colony) will fail.
  4. If you set your wallet address as a recipient, you should see your balance increase by nearly the entire amount of all your payouts (less the network fee).

Scenarios I recommend testing:

  • Multiple payouts
  • Edited payouts (especially removing one of the payouts)
  • Payouts with different claim delays

Note on claim delay in dev

Time works differently in dev blockchain. This makes testing a bit tricky, since the UI uses the real time, whereas the dev blockchain time is "frozen" until the next block is mined.

The consequence of this is that when you finalize an expenditure with non-zero claim delay, the UI should show the correct time at which you can claim the next payout, e.g.:
image

But you won't be able to claim it until another block is mined and the blockchain time updates. To force mining a block, run the following command in truffle:

web3.currentProvider.send({method: "evm_mine", params: []}, () => {})

You can also advance time by an arbitrary number of seconds:

web3.currentProvider.send({method: "evm_increaseTime", params: [<seconds>]}, () => {})
// You need to mine a block after increasing time
web3.currentProvider.send({method: "evm_mine", params: []}, () => {})

Resolves #912

@jakubcolony jakubcolony self-assigned this Aug 7, 2023
@jakubcolony jakubcolony linked an issue Aug 7, 2023 that may be closed by this pull request
@jakubcolony jakubcolony force-pushed the wire/expenditure-funding branch 2 times, most recently from 88ce191 to 6f3afcf Compare August 8, 2023 13:48
Base automatically changed from wire/expenditure-funding to feat/897-expenditure-funding August 8, 2023 13:48
@jakubcolony jakubcolony force-pushed the feat/912-claiming-expenditure branch from 7a24452 to 92d6eee Compare August 8, 2023 14:05
Base automatically changed from feat/897-expenditure-funding to feat/advanced-payments August 10, 2023 19:03
@jakubcolony jakubcolony added the blocked Blocked by another issue label Aug 14, 2023
@jakubcolony
Copy link
Collaborator Author

Marking as blocked by the bug in network - waiting for JoinColony/colonyNetwork#1154 to be merged and deployed

@jakubcolony jakubcolony force-pushed the feat/912-claiming-expenditure branch from 92d6eee to b532bd0 Compare August 18, 2023 13:58
@jakubcolony jakubcolony removed the blocked Blocked by another issue label Aug 21, 2023
@jakubcolony jakubcolony marked this pull request as ready for review August 21, 2023 18:08
@jakubcolony jakubcolony requested review from rdig and willm30 August 21, 2023 18:08
@jakubcolony jakubcolony force-pushed the feat/912-claiming-expenditure branch from 8abb3d4 to 21dc18c Compare August 21, 2023 19:42
Copy link
Contributor

@willm30 willm30 left a comment

Choose a reason for hiding this comment

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

What's the expected behaviour when the recipient is a colony? I am able to click claim on the expenditure (even though I'm not the recipient -- is that right?), but I don't see an incoming funds claim in the colony.

@jakubcolony
Copy link
Collaborator Author

@willm30 The button is supposed to trigger the claim of all claimable payouts, even if you're not the recipient, so that sounds correct

The issue with incoming funds not showing up is most likely due to the fact that we are not storing fund claims from expenditures in the DB yet - I'm about to speak to Raul about this but in the meantime you can verify that the funds have actually been transferred in truffle (after you claim the expenditure):

// the recipient colony
c = await IColony.at()
ta = await c.getToken()
c.claimColonyFunds(ta)

That should update the colony balance.

Thanks for spotting this, it would've gone under the radar otherwise. Everything else should work now so ready for you to have another look 👍

Copy link
Contributor

@willm30 willm30 left a comment

Choose a reason for hiding this comment

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

LGTM. I confirmed the tokens were appearing in the colony I was sending them to, but had to make a normal payment and claim it that way for the funds to be claimed and appear in the UI. Not sure why just doing it in ganache alone wasn't sufficient.

Everything else working as expected 👍 (although testing the claim delay had me scratching my head for a minute 😅 My fault for advancing block time by a few hours!)

((expenditure.finalizedAt ?? 0) + (slot.claimDelay ?? 0)) * 1000,
).getTime();

if (Date.now() >= claimableFrom) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see what you mean about testing this being tricky. I suppose the solution is to only advance block time by a few seconds, so system time can catch up. I guess it's not a problem at all in production since block time can't be meddled with.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can get the actual block time like we do in the fetchMotionTimeoutPeriods lambda but it's an async call and I suppose an anti-pattern for us. And there shouldn't be a difference between block time and date.now() in production, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wanna say it shouldn't make a difference (and the worst case is that the transaction will fail during a small time window) but we'll only know once it's deployed to a real chain

Copy link
Contributor

Choose a reason for hiding this comment

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

🙌

@jakubcolony jakubcolony merged commit 4023ea2 into feat/advanced-payments Aug 23, 2023
@jakubcolony jakubcolony deleted the feat/912-claiming-expenditure branch August 23, 2023 11:10
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.

Claiming an expenditure payout
2 participants