[P1-N08] Converge upon single source of time for all contracts in testing environments #1236

nicholaspai · 2020-04-14T01:59:27Z

Resolves #1047

This PR separates the time storage in Testable.sol from the external methods. This contract requires a lot of down stream changes to tests and scripts because it rejects our previous assumption that each Testable contract maintains its own sense of time.

The major change is that the input bool isTest into Testable.sol is replaced with an address timerAddress. If the address is anything besides the zero address, then the contract is set to a testing environment and Testable.setCurrentTime() can be called on the Timer contract at the input address. If the address passed in is the zero address, then the contract is considered to be in production, and Testable will always return now as the currentTime.

Signed-off-by: Nick Pai <npai.nyc@gmail.com>

core/contracts/common/test/TestableTest.sol

Signed-off-by: Nick Pai <npai.nyc@gmail.com>

nicholaspai · 2020-04-14T15:58:03Z

core/scripts/local/InitializeSystem.js

@@ -90,7 +90,7 @@ const initializeSystem = async function(callback) {
    };
    await tokenizedDerivativeCreator.createTokenizedDerivative(defaultConstructorParams, { from: sponsor });

-    const derivatives = await deployedRegistry.getRegisteredDerivatives(sponsor);
+    const derivatives = await deployedRegistry.getRegisteredContracts(sponsor);


Outside the scope of this PR but this contract method has since been renamed.

core/scripts/cli/sponsor/marketUtils.js

Signed-off-by: Nick Pai <npai.nyc@gmail.com>

mrice32

Looks great! A few minor comments.

mrice32 · 2020-04-14T21:16:29Z

core/contracts/financial-templates/implementation/FeePayer.sol

@@ -63,9 +63,9 @@ abstract contract FeePayer is Testable {
     * @notice Constructs the FeePayer contract. Called by child contracts.
     * @param collateralAddress ERC20 token that is used as the underlying collateral for the synthetic.
     * @param finderAddress UMA protocol Finder used to discover other protocol contracts.
-     * @param isTest whether this contract is being constructed for the purpose of running tests.
+     * @param timerAddress Universal store of time for contracts in test environment.


nit: I would add that this parameter should be set to 0x0 for production environments, so they will fall back to block time. Here and elsewhere.

Good call. Changed to:

* @param _timerAddress Contract that stores the current time in a testing environment. Should be set to 0x0 for production environments that use live time.

One very tiny nit. I would change should to must.

core/migrations/3_deploy_timer.js

mrice32 · 2020-04-14T21:30:36Z

core/migrations/3_deploy_timer.js

@@ -0,0 +1,8 @@
+const Timer = artifacts.require("Timer");


Putting this here because I can't comment on a moved file.

Why move support identifiers from step 10 to step 20 rather than preserve the ordering? The only reason I suggest that we keep the ordering is that they were previously separated into DVM-related migrations (1-10), TD-related migrations (11-16), and EMP-related migrations (17-19). This was to make selective deployments a little easier, specifically, deploying the DVM without any contract creators.

Ah this was a fat finger on my part. I needed to move the Timer migration earlier because its an upstream dependency for a lot of contracts, but accidentally bumped support identifiers to last. Will fix.

Edited order:

Signed-off-by: Nick Pai <npai.nyc@gmail.com>

mrice32

LGTM outside of a tiny nit on the @param comment.

core/contracts/common/implementation/Testable.sol

core/contracts/common/implementation/Timer.sol

chrismaree · 2020-04-15T08:47:10Z

core/contracts/financial-templates/implementation/FeePayer.sol

@@ -63,9 +63,9 @@ abstract contract FeePayer is Testable {
     * @notice Constructs the FeePayer contract. Called by child contracts.
     * @param collateralAddress ERC20 token that is used as the underlying collateral for the synthetic.
     * @param finderAddress UMA protocol Finder used to discover other protocol contracts.
-     * @param isTest whether this contract is being constructed for the purpose of running tests.
+     * @param timerAddress Contract that stores the current time in a testing environment. Should be set to 0x0 for production environments that use live time.


Suggested change

* @param timerAddress Contract that stores the current time in a testing environment. Should be set to 0x0 for production environments that use live time.

* @param timerAddress Contract that stores the current time in a testing environment.

* Should be set to 0x0 for production environments that use live time.

This string is way too long. Can we split them and try keep it below the 120 char limit?

Yup I'll make this change throughout the PR

chrismaree

This looks great! Thanks @nicholaspai, this will really make testing easier into the future. I'm happy except for a few nits on commenting. Please add the natspec to the new Testable files and ensure that the comments you've added to the other contracts are less than 120 chars in length.

Co-Authored-By: Chris Maree <christopher.maree@gmail.com>

Signed-off-by: Nick Pai <npai.nyc@gmail.com>

…ocol into testable-timing

Signed-off-by: Nick Pai <npai.nyc@gmail.com>

nicholaspai · 2020-04-15T16:44:58Z

@mrice32 @chrismaree @ptare Have responde to all comments, any outstanding issues before merging?

nicholaspai added 3 commits April 13, 2020 15:13

separate Testable data store into Timer

06e252d

Signed-off-by: Nick Pai <npai.nyc@gmail.com>

oracle/ tests work

c6591bc

Signed-off-by: Nick Pai <npai.nyc@gmail.com>

financial-templates tests work

26e5aac

Signed-off-by: Nick Pai <npai.nyc@gmail.com>

nicholaspai added the audit-fix-phase1 Fixes for Phase 1 (DVM) of the OZ audit label Apr 14, 2020

mrice32 changed the title ~~[P0-N08] Converge upon single source of time for all contracts in testing environments~~ [P1-N08] Converge upon single source of time for all contracts in testing environments Apr 14, 2020

chrismaree reviewed Apr 14, 2020

View reviewed changes

core/contracts/common/test/TestableTest.sol Outdated Show resolved Hide resolved

nicholaspai added 3 commits April 14, 2020 09:54

Tokenized Derivative tests pass

9829417

Signed-off-by: Nick Pai <npai.nyc@gmail.com>

remove isTest parameter

0102296

Signed-off-by: Nick Pai <npai.nyc@gmail.com>

Merge branch 'master' into testable-timing

850c557

nicholaspai commented Apr 14, 2020

View reviewed changes

core/scripts/cli/sponsor/marketUtils.js Show resolved Hide resolved

nicholaspai added 2 commits April 14, 2020 13:15

fix most of bot tests

1a260a9

Signed-off-by: Nick Pai <npai.nyc@gmail.com>

fix liquidator bug

5e7470c

Signed-off-by: Nick Pai <npai.nyc@gmail.com>

nicholaspai marked this pull request as ready for review April 14, 2020 19:58

test if CI uses different internal clock

a525b93

Signed-off-by: Nick Pai <npai.nyc@gmail.com>

mrice32 reviewed Apr 14, 2020

View reviewed changes

nicholaspai added 6 commits April 14, 2020 18:30

improve timerAddress comment

cf05a7e

Signed-off-by: Nick Pai <npai.nyc@gmail.com>

edited order of migrations

efb8e8e

Signed-off-by: Nick Pai <npai.nyc@gmail.com>

messed up ordering again

85469c2

Signed-off-by: Nick Pai <npai.nyc@gmail.com>

original order + timer inserted at 3

6e6c56b

Signed-off-by: Nick Pai <npai.nyc@gmail.com>

don't create new timer in DeployEMP

52783c8

Signed-off-by: Nick Pai <npai.nyc@gmail.com>

remove uneccessary instantiation

487fc82

Signed-off-by: Nick Pai <npai.nyc@gmail.com>

mrice32 approved these changes Apr 15, 2020

View reviewed changes

chrismaree reviewed Apr 15, 2020

View reviewed changes

core/contracts/common/implementation/Testable.sol Show resolved Hide resolved

chrismaree reviewed Apr 15, 2020

View reviewed changes

core/contracts/common/implementation/Testable.sol Show resolved Hide resolved

chrismaree reviewed Apr 15, 2020

View reviewed changes

core/contracts/common/implementation/Timer.sol Show resolved Hide resolved

chrismaree reviewed Apr 15, 2020

View reviewed changes

core/contracts/common/implementation/Timer.sol Show resolved Hide resolved

chrismaree reviewed Apr 15, 2020

View reviewed changes

chrismaree approved these changes Apr 15, 2020

View reviewed changes

Update core/contracts/common/implementation/Testable.sol

00959d2

Co-Authored-By: Chris Maree <christopher.maree@gmail.com>

nicholaspai and others added 8 commits April 15, 2020 08:35

Update core/contracts/common/implementation/Testable.sol

aac3a15

Co-Authored-By: Chris Maree <christopher.maree@gmail.com>

Update core/contracts/common/implementation/Timer.sol

a5a9cf0

Co-Authored-By: Chris Maree <christopher.maree@gmail.com>

Update core/contracts/common/implementation/Timer.sol

eaee134

Co-Authored-By: Chris Maree <christopher.maree@gmail.com>

comment length

223575d

Signed-off-by: Nick Pai <npai.nyc@gmail.com>

Merge branch 'testable-timing' of https://github.com/UMAprotocol/prot…

693300d

…ocol into testable-timing

comment length

7ee69bb

Signed-off-by: Nick Pai <npai.nyc@gmail.com>

Merge branch 'master' into testable-timing

e23ebe5

fix integration test

4e34420

Signed-off-by: Nick Pai <npai.nyc@gmail.com>

nicholaspai merged commit e9b234f into master Apr 15, 2020

nicholaspai deleted the testable-timing branch April 15, 2020 19:43

mrice32 mentioned this pull request Apr 15, 2020

[P1-L01] Compute late penalty based on current time rather than endTime #1237

Merged

chrismaree mentioned this pull request Apr 27, 2020

[P1-N15] removed import #1299

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[P1-N08] Converge upon single source of time for all contracts in testing environments #1236

[P1-N08] Converge upon single source of time for all contracts in testing environments #1236

nicholaspai commented Apr 14, 2020 •

edited

Loading

nicholaspai Apr 14, 2020

mrice32 left a comment

mrice32 Apr 14, 2020

nicholaspai Apr 14, 2020

mrice32 Apr 15, 2020

mrice32 Apr 14, 2020

nicholaspai Apr 14, 2020

nicholaspai Apr 14, 2020 •

edited

Loading

mrice32 left a comment

chrismaree Apr 15, 2020

chrismaree Apr 15, 2020

nicholaspai Apr 15, 2020

chrismaree left a comment

nicholaspai commented Apr 15, 2020

	* @param timerAddress Contract that stores the current time in a testing environment. Should be set to 0x0 for production environments that use live time.
	* @param timerAddress Contract that stores the current time in a testing environment.
	* Should be set to 0x0 for production environments that use live time.

[P1-N08] Converge upon single source of time for all contracts in testing environments #1236

[P1-N08] Converge upon single source of time for all contracts in testing environments #1236

Conversation

nicholaspai commented Apr 14, 2020 • edited Loading

Choose a reason for hiding this comment

mrice32 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicholaspai Apr 14, 2020 • edited Loading

Choose a reason for hiding this comment

mrice32 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrismaree left a comment

Choose a reason for hiding this comment

nicholaspai commented Apr 15, 2020

nicholaspai commented Apr 14, 2020 •

edited

Loading

nicholaspai Apr 14, 2020 •

edited

Loading