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

Feat/restituton2.0 #1

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Feat/restituton2.0 #1

wants to merge 8 commits into from

Conversation

sajanrajdev
Copy link
Collaborator

Patched remBADGER according to the requirements for the restitution program continuation. Specs: https://docs.google.com/document/d/14Ub20r21C7Sqt05SeMyjRX4Qr76PxpeWPLZpC91GYHw/edit

TL'DR of requirements
Patch remBADGER to allow for one user to deposit with an specific amount of BADGER. Relevant amendment: https://snapshot.org/#/badgerdao.eth/proposal/0xf920921a610aa3f50581e5cc9c0e505c2f45e19541a88dfeafffcaba3757416c

Changes introduced:

  • Removed the governance only modifier to the remBADGER's deposit function
  • Introduces governance only function to re-enable deposits on remBADGER
  • By themselves, these changes would allow any user to deposit when triggered. Therefore, a guestlist should be added and be present at the moment of enabling deposits.
  • The production Guestlist contract was added to the repo. The deposit cap accounting functions were wrong in this version and were fixed.
  • Fixed the brownie version to the one used back then as newer versions introduced breaking changes.
  • Added an integration test for the whole process of allowing this single deposit, run with:
brownie test tests/test_rembadger_amendment_upgrade.py

@sajanrajdev
Copy link
Collaborator Author

@dapp-whisperer @wtj2021

As per @GalloDaSballo's suggestion, I simplified the approach significantly by keeping the governanceOnly check on the deposit function and having the governance deposit on behalf and for the user using depositFor. This removes the need of a guestlist and allow for the full process to be performed atomically given that the user transfers the amount to the governance multisig prior to execution. The test suite was modified accordingly.

    Atomic operation:
    1. Execute the upgrade to the new implementation
    2. Open deposits for governance only
    3. Governance approves the deposit amount to remBADGER
    4. Governance calls depositFor the whitelisted user
    5. Governance bricks deposits

Thanks @GalloDaSballo !!!

)

# User transfers the amount of BADGER to deposit to governance
badger.transfer(devMultisig, WHITELISTED_AMOUNT, {"from": whitelisted_user})

Choose a reason for hiding this comment

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

@sajanrajdev This transfer is just for test?

You can make it atomic by doing transferFrom

But it's simpler if the user just sends the tokens beforehand

@GalloDaSballo
Copy link

@sajanrajdev The changes LG, note that you need the user to either approve for a transferFrom or they have to send the tokens earlier

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.

3 participants