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 mainnet forking test case #4306

Merged
merged 20 commits into from
Dec 20, 2022

Conversation

chrismaree
Copy link
Member

@chrismaree chrismaree commented Dec 13, 2022

Motivation

We want the ability to write mainnet fork tests to validate some core DVM behavior. This PR adds this ability the protocol Repo and adds a sample test case that enques a new request, stakes new UMA, votes on the request, resolves the request and unstakes the UMA. All of this is done against a fork.

To run this test use: forge test --match-path *fork-test* -vvv --fork-url $CUSTOM_NODE_URL where CUSTOM_NODE_URL is either a goerli or mainnet address (mainnet not supported just yet).

Note this functionality is designed to work _in conjunction with existing foundry tests. if the node URL is not configured then the unit tests will skil the mainnet forking tests.

Signed-off-by: chrismaree <christopher.maree@gmail.com>
Signed-off-by: chrismaree <christopher.maree@gmail.com>
Signed-off-by: chrismaree <christopher.maree@gmail.com>
Signed-off-by: chrismaree <christopher.maree@gmail.com>
Signed-off-by: chrismaree <christopher.maree@gmail.com>
Signed-off-by: chrismaree <christopher.maree@gmail.com>
vm.prank(registeredRequester);
assertEq(voting.getPrice(identifier, requestTime, ancillaryData), price);

// Can unstake and has more than started with due to rewards and positive slashing.
Copy link
Contributor

Choose a reason for hiding this comment

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

This depends on having other accounts having staked on fork time. If its 0 (e.g. shortly after deployment) then this test might fail as there are no positive slashing if TestAddress.account1 is the sole staker.

Copy link
Contributor

@Reinis-FRP Reinis-FRP Dec 14, 2022

Choose a reason for hiding this comment

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

Also note that the test does not withdraw rewards, so any balance increase is only due to positive slashing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just for the sake of robustness could add a TestAddress.account2 as staker not participating in vote so that there always is positive slashing for TestAddress.account1

Copy link
Member Author

Choose a reason for hiding this comment

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

we'll only ever run this against a deployment that has been setup and is actually being used I'd say. so worrying about there being no other stakers is less of an issue. I do agree, though, that the test should separately validate that you can: 1) get rewards (calling the claim reward function) and 2) withdraw from unstake.

Signed-off-by: chrismaree <christopher.maree@gmail.com>
voting.requestUnstake(voting.getVoterStakePostUpdate(TestAddress.account1));
vm.warp(voting.getCurrentTime() + voting.unstakeCoolDown());
voting.executeUnstake();
assert(votingToken.balanceOf(TestAddress.account1) > gatMeetingAmountOfTokens);
assert(votingToken.balanceOf(TestAddress.account1) > stakerBalanceAfterRewardWithdrawal);
Copy link
Contributor

@Reinis-FRP Reinis-FRP Dec 14, 2022

Choose a reason for hiding this comment

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

stakerBalanceAfterRewardWithdrawal accounts only for rewards, so this check should add gatMeetingAmountOfTokens and subtract stakerBalanceBeforeRewardWithdrawal on the right side

Copy link
Member Author

Choose a reason for hiding this comment

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

good point. have updated

@@ -75,7 +75,7 @@
"sideEffects": false,
"scripts": {
"test": "yarn hardhat-test && yarn forge-test",
"forge-test": "./scripts/SetUpFoundryWithHardhat.sh && forge test",
Copy link
Contributor

Choose a reason for hiding this comment

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

this breaks CI with error Command "forge-test" not found

Copy link
Member Author

Choose a reason for hiding this comment

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

have addressed.

Signed-off-by: chrismaree <christopher.maree@gmail.com>
Signed-off-by: chrismaree <christopher.maree@gmail.com>
Copy link
Member

@mrice32 mrice32 left a comment

Choose a reason for hiding this comment

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

Looks good! Only minor comments

@@ -0,0 +1,53 @@
// SPDX-License-Identifier: UNLICENSED
Copy link
Member

Choose a reason for hiding this comment

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

tiny nit: doesn't this disagree with our repo license?

Comment on lines 26 to 27
shouldRunTest = (chainId == 1 || chainId == 5);
if (!shouldRunTest) return; // Exit early if we are not executing forked tests.
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Is this meant to house any DVM tests? Or just specifically forked ones?

I noticed that shouldRunTest is based on whether we're on a fork, but everything else in here seems general. Thoughts on renaming this variable to shouldRunForkTests?

Copy link
Member Author

Choose a reason for hiding this comment

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

nope, this is meant to house only fork-tests, as it is located in this directory. agreed though that the variable name is not as explicit as it should be. I'll update it.

if (!shouldRunTest) return; // Exit early if we are not executing forked tests.

// TODO: look into a way to not have to hard code these addresses. Ok for now as we wont be changing them.
address votingAddress = chainId == 1 ? address(0) : 0xF71cdF8A34c56933A8871354A2570a301364e95F;
Copy link
Member

Choose a reason for hiding this comment

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

Assuming there's also a TODO here for replacing this address(0) with the real deployment address?

// Move to next round and vote from the newly staked account.
moveToNextRound();
int256 price = 1e18;
int256 salt = 42069;
Copy link
Member

Choose a reason for hiding this comment

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

Can't believe this number popped out of your local rng +1

@@ -396,24 +396,24 @@ describe("ServerlessHub.js", function () {
MONITOR_CONFIG: { contractVersion: "2.0.1", contractType: "ExpiringMultiParty" },
},
},
testServerlessLiquidator: {
serverlessCommand: "yarn --silent liquidator --network test",
testServerlessMonitor2: {
Copy link
Member

Choose a reason for hiding this comment

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

What is this change? Was this meant to go into this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

this was unrelated...sorry I was working on multiple PRs at the same time and this was changed due to this PR: #4309

I've reverted

uint256 stakerBalanceBeforeRewardWithdrawal = votingToken.balanceOf(TestAddress.account1);
voting.withdrawRewards();
uint256 stakerBalanceAfterRewardWithdrawal = votingToken.balanceOf(TestAddress.account1);
assert(stakerBalanceAfterRewardWithdrawal > stakerBalanceBeforeRewardWithdrawal);
Copy link
Member

Choose a reason for hiding this comment

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

nit: could be a stronger test if we can change this and the assertion on line 59 to equality assertions rather than inequality to be sure the value is exactly what we expect.

For instance, if the contract was broken and sent you 10x the tokens in rewards or unstakes, these checks would all pass.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with @mrice32 suggestion

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

Signed-off-by: chrismaree <christopher.maree@gmail.com>
Copy link
Contributor

@md0x md0x left a comment

Choose a reason for hiding this comment

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

LGTM! Very cool this usage of foundry
Agree with the teams' suggestions - particularly the offchain calculation of the stake rewards to make the test more detailed.

@chrismaree chrismaree merged commit 48127c6 into master Dec 20, 2022
@chrismaree chrismaree deleted the chrismaree/forked-test-enviroment branch December 20, 2022 16:27
Reinis-FRP pushed a commit that referenced this pull request Dec 28, 2022
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
Reinis-FRP added a commit that referenced this pull request Jan 10, 2023
* feat: implement dvm v2 unstake monitor

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* feat: implement dvm v2 stake monitor

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* feat: implement dvm v2 proposal monitor

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* feat: implement dvm v2 deletion proposal monitor

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* feat: implement dvm v2 emergency proposal monitor

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* feat: implement dvm v2 rolled votes monitor

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix dvm monitor logger imports

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* feat: update dvm2.0 upgrade script (#4304)

* feat: update dvm2.0 upgrade script

Signed-off-by: Pablo Maldonado <pablo@umaproject.org>

* feat: update starting index

Signed-off-by: Pablo Maldonado <pablo@umaproject.org>

* refactor: update comments

Signed-off-by: Pablo Maldonado <pablo@umaproject.org>

* feat: update starting index and starting id

Signed-off-by: Pablo Maldonado <pablo@umaproject.org>

* fix: governor starting id

Signed-off-by: Pablo Maldonado <pablo@umaproject.org>

* fix: add previous voting contracts events

Signed-off-by: Pablo Maldonado <pablo@umaproject.org>

* fix: add previous voting contracts events docs

Signed-off-by: Pablo Maldonado <pablo@umaproject.org>

* refator: improve comment

Signed-off-by: Pablo Maldonado <pablo@umaproject.org>

Signed-off-by: Pablo Maldonado <pablo@umaproject.org>
Co-authored-by: Pablo Maldonado <pablo@umaproject.org>
Co-authored-by: chrismaree <christopher.maree@gmail.com>
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix: address issue in CI (#4315)

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* build(deps): bump decode-uri-component from 0.2.0 to 0.2.2 (#4301)

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: chrismaree <christopher.maree@gmail.com>
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* chore: Bump ethers to 5.7.x in common and sdk (#4214)

Co-authored-by: chrismaree <christopher.maree@gmail.com>
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* feat: add new health check runner package (#4311)

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* improve: Add MerkleTree improvements made in across-protocol/contracts-v2 (#4318)

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* feat: new release (#4322)

* feat: new release

Captures MerkleTree util change to match across-protocol/contracts-v2

* Update CHANGELOG.md

* Update CHANGELOG.md

* lint

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* feat: Add mainnet forking test case (#4306)

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix[OA-audit-L03]: Address erronous docstrings and comments (#4320)

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix[OA-audit-N04]: Add additional index paramaters (#4323)

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix[OA-audit-N05]: Address argument name mismatches between interface and implementation (#4324)

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix[OA-audit-N06]: Address Missing event parameter (#4325)

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix[OA-audit-N07]: Fix licencing issues accross the OA (#4326)

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix[OA-audit-N08]: Fix address multicaller payability (#4327)

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix[OA-audit-N09]: Missing check for non-zero address (#4336)

Co-authored-by: Pablo Maldonado <pablo@umaproject.org>
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix: use external functions in oa (#4337)

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix: remove unused import (#4338)

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix: dont use named return variables (#4341)

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix[OA-audit-N10]: Fix redundant code (#4335)

* fix: remove override from base em

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix: shared getRequestId function in full policy em

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix: simplify require in configureEscalationManager

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix: simplify em lookup in oa

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix: store callback recipients as local variables in oa

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix: redundant return in oa

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix: remove forge-std lib

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix: add back gitmodules

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix: add newline in gitmodules

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix: newline in gitmodules

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix: shorten variable names

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix[OA-audit-N12]: Typographical errors (#4339)

Signed-off-by: Pablo Maldonado <pablo@umaproject.org>

Signed-off-by: Pablo Maldonado <pablo@umaproject.org>
Signed-off-by: chrismaree <christopher.maree@gmail.com>
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
Co-authored-by: Pablo Maldonado <pablo@umaproject.org>
Co-authored-by: chrismaree <christopher.maree@gmail.com>
Co-authored-by: Reinis Martinsons <reinis@umaproject.org>
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix[OA-audit-L01]: Limit ability for Full Policy Escalation Manager to setArbitrationResolution (#4319)

* fix: Limit ability for Full Policy EscilationManager to setArbitrationResolution

Signed-off-by: chrismaree <christopher.maree@gmail.com>

* fix

Signed-off-by: chrismaree <christopher.maree@gmail.com>

* Update packages/core/test/foundry/optimistic-asserter/escalation-manager/FullPolicyEscalationManager.t.sol

Signed-off-by: chrismaree <christopher.maree@gmail.com>
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
Co-authored-by: Reinis Martinsons <77973553+Reinis-FRP@users.noreply.github.com>
Co-authored-by: Reinis Martinsons <reinis@umaproject.org>
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix[OA-audit-L02]: Lack of access control (#4334)

* fix[OA-audit-L02]: Lack of access control on potentially sensitive function

Signed-off-by: Pablo Maldonado <pablo@umaproject.org>

* test: test onlyOptimisticAsserter modifier

Signed-off-by: Pablo Maldonado <pablo@umaproject.org>

* fix

Signed-off-by: chrismaree <christopher.maree@gmail.com>

* Update packages/core/test/foundry/optimistic-asserter/escalation-manager/DisputeLimitingEscalationManager.t.sol

Co-authored-by: Reinis Martinsons <77973553+Reinis-FRP@users.noreply.github.com>

* Update packages/core/test/foundry/optimistic-asserter/escalation-manager/BaseEscalationManager.t.sol

Co-authored-by: Reinis Martinsons <77973553+Reinis-FRP@users.noreply.github.com>

* Update packages/core/test/foundry/optimistic-asserter/escalation-manager/DisputeLimitingEscalationManager.t.sol

Co-authored-by: Reinis Martinsons <77973553+Reinis-FRP@users.noreply.github.com>

* fix

Signed-off-by: chrismaree <christopher.maree@gmail.com>

* fix: remove Ownable from base em

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix: reorder imports

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

Signed-off-by: Pablo Maldonado <pablo@umaproject.org>
Signed-off-by: chrismaree <christopher.maree@gmail.com>
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
Co-authored-by: Pablo Maldonado <pablo@umaproject.org>
Co-authored-by: chrismaree <christopher.maree@gmail.com>
Co-authored-by: Reinis Martinsons <77973553+Reinis-FRP@users.noreply.github.com>
Co-authored-by: Reinis Martinsons <reinis@umaproject.org>
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix[OA-audit-N15]: Use of magic numbers (#4340)

Co-authored-by: Pablo Maldonado <pablo@umaproject.org>
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix[OA-audit-L04]: Add missing or incomplete docstrings (#4342)

* fix: natspec in oa public methods

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix: add docstrings to oa interface

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix: add natspec to oa callback recipient interface

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix: natspec in base em

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix: natspec em interface

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix: natspec full policy em

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix:

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix: add comments to internal oa functions

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix[OA-audit-N03]: Always send oracle fee to the Store (#4343)

* fix: always pay oracle fee

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix: lift zero fee check

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* feat: refactor dmv monitoring to use single entry

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* feat: add readme

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix: readme

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* feat: monitor governor transfers

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* feat: monitor uma mints

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix: rename common file

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* Revert "fix: rename common file"

This reverts commit e674408.

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* feat: rename dvm monitor

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix: improve block update and add comments

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix: add token units to readme

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix: parse threshold parameters from main entry point

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix: update block range only in main entry point

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix: update log levels

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix: typescript error

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* feat: use tsc

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix: update readme

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix: monitor bots after dvm refactor

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix:

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix:

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix:

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix:

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* fix:

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
Signed-off-by: Pablo Maldonado <pablo@umaproject.org>
Signed-off-by: chrismaree <christopher.maree@gmail.com>
Co-authored-by: Pablo Maldonado <pablomaldonadoturci@gmail.com>
Co-authored-by: Pablo Maldonado <pablo@umaproject.org>
Co-authored-by: chrismaree <christopher.maree@gmail.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: nicholaspai <9457025+nicholaspai@users.noreply.github.com>
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.

4 participants