search

DemocracyEarth / ubi

Universal Basic Income token.
224 stars 39 forks source link

ERC20 stream features for UBI #87

Closed 0xc0de4c0ffee closed 3 years ago

0xc0de4c0ffee commented 3 years ago

Namaste, There's some room for optimizations and splitting big functions like startStream into separate streamRequest and streamApprove functions.

I'm still not sure about possible upgrade patterns and transition functions required, so give it a try.

santisiri commented 3 years ago

I'll inspect these changes by merging them on the streaming branch.

michaelwoodson commented 3 years ago

I posted my thoughts reading through this PR since @santisiri mentioned wanting someone to review it on Telegram. This will be a very very useful change, thanks for posting it!

santisiri commented 3 years ago

Great review @michaelwoodson! Much appreciated, I formally turned this also into HIP-6 proposal on the DAO: https://gov.proofofhumanity.id/t/hip-6-enhance-streaming-features-for-ubi/394

@0xc0de4c0ffee I think it makes sense to split this into smaller PRs.. that would increase chances of getting this merged on chain. Also, I wouldn't include changeAccruedRate since this could definitely introduce a lot of complexity moving forward and should be a HIP of its own.

michaelwoodson commented 3 years ago

Thanks again for posting this PR @0xc0de4c0ffee . Over the last few days I did some more testing locally and posted a few more notes just now based on that. Main thing is to think about how to transition to the activated flags.

I was curious about the performance implications of different approaches so I tried them out locally and I went ahead and pushed those to github for reference. There is a minimal branch with the bare minimum changes to get streaming working, totalsupply adds the totalSupply optimization, and there is an original branch to compare to the unstreamed version. The minimal and totalsupply branches tweak the api, you pass in a percentage of the stream and the contract manages the recipients so the api for removeRecipient isn't broken. There is also a strategy (streamedSince instead of activated) for upgrading. Shared it in case it is useful, I saw that you are already working on a similar change moving most of the variables into a struct.

https://github.com/michaelwoodson/ubi

santisiri commented 3 years ago

Since we are looking at a different implementation for streaming with less risk in the modifications introduced, I'll proceed to close this PR. Still uncertain if we can get this done saferly at the token contract level.