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

refactor(StakeManager): optimize stake, account init and lock function #130

Merged
merged 5 commits into from
Oct 13, 2024

Conversation

3esmit
Copy link
Collaborator

@3esmit 3esmit commented Sep 23, 2024

Description

This pull request focuses solely on cleaning up the code. The clean-up removes outdated or unnecessary code, which was masking a bug that is being addressed in #127

Details

This changes the account initialization on stake():

This changes the method lock():

Details on #127 bug

The bug was hidden because the _mintBonusMP function contained a line that updated account.lastMint to the same value as startEpoch. This had the unintended effect of making the epoch estimation appear correct when calling StakeManager.lock(). As a result, the underlying bug wasn’t detected in test_UpdateLockupPeriod.

PR #130 does not fix this bug; instead, it cleans up the code that was hiding it for test_UpdateLockupPeriod to cach it. The actual bug fix is handled in PR #127, which also introduces other tests that would have catched it while implementing the multiplier points estimation.

Purpose

Code Cleanup: This PR removes code that was masking the bug, allowing the system to function properly moving forward.
Not a Bug Fix: It doesn't directly fix the bug but removes the part of the code that was preventing the issue from being caught in the test.

Important Notes

The test failures seen in executeAccount->processAccount->mintMP are unrelated to this cleanup. However, PR #130 requires that PR #127 and PR #129 be merged first to ensure proper function.

Checklist

Ensure you completed all of the steps below before submitting your pull request:

  • Added natspec comments?
  • Ran forge snapshot?
  • Ran pnpm gas-report?
  • Ran pnpm lint?
  • Ran forge test?
  • Ran pnpm verify?

@3esmit 3esmit self-assigned this Sep 24, 2024
@0x-r4bbit
Copy link
Collaborator

Changes look good to me, but it seems this is not passing tests

@3esmit
Copy link
Collaborator Author

3esmit commented Sep 25, 2024

Updated description to explain exactly whats happening here, why this tests are failing for this correct code change.

@3esmit
Copy link
Collaborator Author

3esmit commented Sep 27, 2024

So this PR is failing on verify because the spec is outdated.

Two things are failing:

@3esmit 3esmit changed the title refactor(StakeManager): optimize stake and lock functionality by remo… refactor(StakeManager): optimize stake, account init and lock function Sep 27, 2024
@0x-r4bbit
Copy link
Collaborator

@3esmit I added a commit here with these changes:

This should do the trick

@3esmit 3esmit force-pushed the remove-mintBonusMP-function branch from c2a4851 to 254ea0a Compare October 3, 2024 03:24
3esmit and others added 5 commits October 12, 2024 17:35
change

The rule is failing since we've removed the `lockUntil > 0` check in
`stake` and `processAccount` is no longer used in `stake()`.
The rule requires `lockUntil > 0` so it will always fail there and can't
find a non-reverting cases (which makes the rule pass).

The reason it hasn't happened before was because:

The rule required account.lockUntil > 0
Stake required lockUntil > 0 || account balance == 0

Also this commit adds an invariant:

add invariant that account balance == 0 => accountMP == 0
This invariant failed as the prover started making wrong assumptions
about the relationship between anyone's account's `totalMP` and its
`balance`, as well as an account's `bonusMP` and its `balance`.

This commit fixes it by adding the necessary invariants to proof the
property.
@3esmit 3esmit force-pushed the remove-mintBonusMP-function branch from 254ea0a to e5d9a45 Compare October 12, 2024 20:39
@3esmit 3esmit merged commit c0bf36d into develop Oct 13, 2024
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants