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

Add ExpenditurePayoutClaimed event #1157

Merged
merged 3 commits into from
Sep 8, 2023

Conversation

area
Copy link
Member

@area area commented Aug 21, 2023

From @jakubcolony

I think I'll need to store whether a payout has been already claimed or not, to be able to determine whether there's any more payouts to claim from the UI. I thought about listening to the PayoutClaimed event in block-ingestor and then checking whether the funding pot ID belongs to an expenditure. However, this event doesn't contain information on which slot has been claimed, just the token address and the amount, which I don't think is enough to uniquely identify the claimed payout.

I suppose I could call the getExpenditureSlotPayout every time I want to check their claimed status but caching it is a more preferable solution. Do you have any ideas on how that could be done? Could we possibly have another expenditure specific event, say ExpenditurePayoutClaimed, that would contain the slot info?

I agreed with Jakub's conclusion that there is no way to uniquely identify a claimed payout (especially if there are multiple payouts for the same amount / destination), short of tracing the transaction which we really don't want to expect people to to, and agree that the easiest way to solve this is to add the ExpenditurePayoutClaimed event.

There's some crossover here with #1150; when that is complete there will only be one type of payout on the core contracts, so perhaps with this merged, the even there could go back to PayoutClaimed with an adjusted signature; alternatively, we could start this event named PayoutClaimed which would be a little confusing to begin with but correct moving forward.

contracts/colony/ColonyDataTypes.sol Outdated Show resolved Hide resolved
contracts/colony/ColonyFunding.sol Outdated Show resolved Hide resolved
@area area force-pushed the feat/expenditure-payout-event branch from c5cc8e5 to 19efb73 Compare August 23, 2023 09:56
@area
Copy link
Member Author

area commented Aug 23, 2023

I will note that, the way that this is currently structured, the payout value is different for these two events (see the test). One includes the network fee, and one does not. Should the new event use the same value as the old one?

@kronosapiens
Copy link
Member

@area hmm... that's an interesting question. I'm inclined to say the event should be the larger number (i.e. include the fee), since that would make it possible to use the event to track current balances and will probably be more useful for a UI.

@area
Copy link
Member Author

area commented Aug 24, 2023

How are you envisaging using the event to track current balances? Can't the current balance of whatever you're interested in just be queried?

@area area force-pushed the feat/expenditure-payout-event branch from 19efb73 to 056a696 Compare August 24, 2023 15:54
@kronosapiens
Copy link
Member

@area sure, but if the intention is to reduce the need for queries then being able to prepare a UI using the event log seems advantageous. Also, you can use the underlying ERC20 transfer event if you need to get the actual amount the user received. Emitting the value prior to the network key will keep the expenditure-related events to be internally consistent, i.e. agnostic to the network fee.

@area area force-pushed the feat/expenditure-payout-event branch from 056a696 to ebf7627 Compare September 8, 2023 14:35
@kronosapiens kronosapiens merged commit d2086e9 into develop Sep 8, 2023
@kronosapiens kronosapiens deleted the feat/expenditure-payout-event branch September 8, 2023 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants