search

DemocracyEarth / ubi

Universal Basic Income token.
224 stars 39 forks source link

UBI Token Review #36

Closed fnanni-0 closed 3 years ago

fnanni-0 commented 3 years ago

https://docs.google.com/document/d/1FHmn5Xwk8ECfjV76YiqCNj39uLGNyBLd3qt-OWHdOs4/edit?usp=sharing

santisiri commented 3 years ago

Thanks @fnanni-0, looking into the review right now.

santisiri commented 3 years ago

Changes implemented here: https://github.com/DemocracyEarth/ubi/pull/39

Kept the simplistic approach yet I think in the future we could improve accrual mechanism with something analogue to how rebasing works in Ampleforth

fnanni-0 commented 3 years ago

Beware that the current snapshot id is not visible to external contracts directly. You can only access it by querying the Snapshot event. I'm not sure if this could become an issue in the future. You might want to make the snapshot() function return the current id, and maybe even store it.

santisiri commented 3 years ago

Hi @fnanni-0 , took your advice and implemented here.. looks like after all is probably better to bring upfront the functionality of snapshot itself to the main contract. check the changes here: https://github.com/DemocracyEarth/ubi/compare/issuance

fnanni-0 commented 3 years ago

@santisiri that won't work because you can't override the private variable _currentSnapShortId.

I think it would be enough to just return the new id:

  function snapshot() external onlyByGovernor returns(uint256) {
    return _snapshot();
  }
santisiri commented 3 years ago

Ah.. I see.. now that makes sense. Thanks @fnanni-0, will fix and merge now.