Skip to content

Conversation

danoctavian
Copy link
Contributor

@danoctavian danoctavian commented Jan 17, 2023

Context

Issue

#607

#592

Changes proposed in this pull request

Enable the steps of the fork test that transfer governance rewards out to owners, process the claim assesment staked tokens for all assessors and send them to the owners with a permisionless call and measures that expected token balances are in the right amount.

Fix memory layout for TokenController.

Fix ability to receive NXM tokens through operator transfer for TokenController.

add script to measure all withdrawable cover notes to account for tokens present in TokenController.

scripts/get-withdrawable-cover-notes.js

Test plan

The ticket concerns the fork test in itself, no other testing is required.

Checklist

  • Rebased the base branch
  • Attached corresponding Github issue
  • Prefixed the name with the type of change (i.e. feat, chore, test)
  • Performed a self-review of my own code
  • Followed the style guidelines of this project
  • Made corresponding changes to the documentation
  • Didn't generate new warnings
  • Didn't generate failures on existing tests
  • Added tests that prove my fix is effective or that my feature works

Review

When reviewing a PR, please indicate intention in comments using the following emojis:

  • 🍰 = Nice to have but not essential.
  • 💡 = Suggestion or a comment based on personal opinion
  • 🔨 = I believe this should be changed.
  • 🤔 = I don’t understand something, do you mind giving me more context?
  • 🚀 = Feedback

coverNotesSum: coverNotesSum.toString(),
});

// TODO: this does NOT pass. Find out where the extra 7k tokens is from.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Proposal:

Merge this as is.

Finish up staking migration to wrap all aspects up and clear all unknowns, then get back to this issue.

The fact that there's extra tokens makes it less bad than the situation when tokens would be missing.

console.log({
governanceRewardsMigrated: governanceRewardsMigrated.toString(),
});
// expect(governanceRewardsMigrated).to.be.equal(this.governanceRewardsSum);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a discrepancy here as well on the side of excess tokens. by a slight amount.

  -1106654884061072517264
  +870391213513961173071
  
  
  Extra tokens:
  
  236.26367054711136 NXM

@danoctavian danoctavian force-pushed the feature/v2-migration-fork-test branch from 54f7e01 to dc2c977 Compare January 19, 2023 17:16
@danoctavian danoctavian changed the base branch from feature/v2-migration-fork-test to nexus-v2 January 19, 2023 17:31
@danoctavian danoctavian force-pushed the feature/v2-migration-claim-assessment-migration branch from fb54e9f to 057ee27 Compare January 19, 2023 19:50
process.exit(1);
});
if (require.main === module) {
const provider = new ethers.providers.JsonRpcProvider(process.env.PROVIDER_URL);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's import ethers from hardhat and here and pass ethers.provider to the function instead - this would make the script "hardhat compatible" as you'd be able to hh run --network somenetwork scripts/get-governance-rewards.js. As a side note, the reason this function accepts a provider instead of using ethers.provider directly is because you'd be able to pass a direct provider, for example from a fork test. Currently the skipped test inside the fork passes ethers.provider which is not the intended use of the function.

Copy link
Contributor

Choose a reason for hiding this comment

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

LE: fixing myself

@danoctavian
Copy link
Contributor Author

Screen Shot 2023-01-22 at 6 52 56 PM

works all except pool-value as expected merging

@danoctavian danoctavian force-pushed the feature/v2-migration-claim-assessment-migration branch from 64dc2a5 to 0b543a9 Compare January 22, 2023 16:54
@danoctavian
Copy link
Contributor Author

reran after rebase and WETH update.

Screen Shot 2023-01-22 at 6 58 37 PM

@danoctavian danoctavian merged commit 2a28c30 into nexus-v2 Jan 22, 2023
@danoctavian danoctavian deleted the feature/v2-migration-claim-assessment-migration branch January 22, 2023 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants