[ETHEREUM-CONTRACTS] don't claim as a side effect of updateMemberUnits()

superfluid-finance / protocol-monorepo

Superfluid Protocol Monorepo: the specification, implementations, peripherals and development kits.

https://www.superfluid.finance

Other

875 stars 239 forks source link

[ETHEREUM-CONTRACTS] don't claim as a side effect of updateMemberUnits() #1883

Closed d10r closed 8 months ago

d10r commented 8 months ago

Currently pool.updateMemberUnits() does also claimAll(). I have commented before that I consider that to be a bad idea, as it's an unexpected side effect and imo has more cons than pros.

I sense no opposition against this change and think it should be done, thus attempting a PR.

PS: some tests are failing. Before spending time on those I want to understand if we agree on making this change.

github-actions[bot] commented 8 months ago

Changelog Reminder

Reminder to update the CHANGELOG.md for any of the modified packages in this PR.

[ ] CHANGELOG.md modified
[ ] Double check before merge

kasparkallas commented 8 months ago

What about doing it for the pool member only when the units go to 0?

d10r commented 8 months ago

What about doing it for the pool member only when the units go to 0?

Dunno, that would just replace one kind of special case with another one.

I'd instead consider adding some configurability around claim, e.g. to make it permissioned by the receiver, maybe with an option for the admin to recover unclaimed funds. But that's out of scope of this PR.

github-actions[bot] commented 8 months ago

XKCD Comic Relif

Link: https://xkcd.com/1883