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: add tests after ownership is transfer to assert behaviours #1395

Merged
merged 5 commits into from
Jan 24, 2024

Conversation

petrovska-petro
Copy link
Collaborator

Tackles latest reccoms in #1277

Copy link
Collaborator

@sajanrajdev sajanrajdev left a comment

Choose a reason for hiding this comment

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

Tests showed that all governance functions will be executable from the Timelock and that the BADGER token will remain fully functional (as expected).

NOTE: The BADGER minting processes will require two separate timelock operations:

  • Minting
  • Transfering the minted tokens out of the Timelock and into their destination

CC @dapp-whisperer, Petro has now pushed the tests for this operation as per your request. Please take a look and note the above.

@sajanrajdev
Copy link
Collaborator

These tests revealed the need for a redesign of the approach as the Timelock would now become the recipient of any minted BADGER which isn't an intended behavior. Nevertheless, there is value on preserving these tests and I would say we move for a merge of them.

@rayeaster, would you mind taking a look?

Copy link

@rayeaster rayeaster left a comment

Choose a reason for hiding this comment

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

LGTM with minor comment

tl_gov = accounts.at(r.governance_timelock, force=True)

# mint
controller.mint(100e18, {"from": tl_gov})

Choose a reason for hiding this comment

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

could use a variable to hold the magic value of 100e18


badger.transfer(holder_two, 5e18, {"from": holder_one})

assert badger.balanceOf(holder_two) == holder_two_bal_before + 5e18

Choose a reason for hiding this comment

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

could use a variable to hold the magic value of 5e18

["address", "uint256"],
[safe.address, int(100e18)],
)
eta = chain.time() + int(3 * 60 * 60 * 24)

Choose a reason for hiding this comment

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

could use a variable to hold the magic value of 3 * 60 * 60 * 24

@sajanrajdev
Copy link
Collaborator

Good comments with regards the magic values @rayeaster. Given that these tests are more informative at this point and the operation is not planned anymore, I will go ahead and merge as is.

@sajanrajdev sajanrajdev merged commit 15f02b2 into main Jan 24, 2024
3 checks passed
@sajanrajdev sajanrajdev deleted the feat/owner-controler-test branch January 24, 2024 23:37
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