Utilizes rollfork to obsolete deploy-info script #166

brianmcmichael · 2023-02-11T01:13:51Z

Description

Builds upon the rollFork innovation to use binary search for deployed block.

src/DssSpell.t.base.sol

naszam

LGTM

iamchrissmith · 2023-02-13T20:24:46Z

src/DssSpell.t.base.sol

+        }
+
+        uint256 low = 5273074; // MCD_VAT Deployed Aug-06-2021 04:26:38 PM +UTC
+        uint256 high = block.number;


Suggested change

uint256 high = block.number;

uint256 max = block.number;

iamchrissmith · 2023-02-13T20:24:51Z

src/DssSpell.t.base.sol

+            return;
+        }
+
+        uint256 low = 5273074; // MCD_VAT Deployed Aug-06-2021 04:26:38 PM +UTC


I see why you picked this reference point, but why not pick a recent block. We know that this search really only needs to look at blocks for future spells, right?

Yeah, taking into account this is for future spells we could put a recent block here.

I created this to address this issue: #168

Block is where the vat was deployed on goerli. Binary search is O(log n) so it doesn't matter whether we use this or block 0, the search time will not differ much.

iamchrissmith · 2023-02-13T20:24:59Z

src/DssSpell.t.base.sol

+            return;
+        }
+
+        uint256 low = 5273074; // MCD_VAT Deployed Aug-06-2021 04:26:38 PM +UTC


Suggested change

uint256 low = 5273074; // MCD_VAT Deployed Aug-06-2021 04:26:38 PM +UTC

uint256 min = 5273074; // MCD_VAT Deployed Aug-06-2021 04:26:38 PM +UTC

I avoided max and min due to higher possibilities of collision with global variables.

iamchrissmith · 2023-02-13T20:45:17Z

src/DssSpell.t.base.sol

+                // we want to roll our fork to the block where it was deployed
+                // this means the test suite will continue to accurately pass/fail
+                // even if mainnet has already scheduled/cast the spell


Suggested change

// we want to roll our fork to the block where it was deployed

// this means the test suite will continue to accurately pass/fail

// even if mainnet has already scheduled/cast the spell

// we want to roll our fork to a block after it was deployed and

// before it was scheduled this means the test suite will continue

// to accurately pass/fail even if goerli has already

// scheduled/cast the spell

gbalabasquer · 2023-02-14T15:39:05Z

src/DssSpell.t.base.sol

+    }
+
+    function _rollToDeployedSpellBlock(address _spell) internal {
+        if (_spell == address(0)) {


Not sure I understand this part. The != address(0) check is already happening before calling this function.

Reusability.

A general version of this could probably be moved into dss-test

gbalabasquer · 2023-02-14T16:01:33Z

src/DssSpell.t.base.sol

+        uint256 mid;
+
+        while (low < high) {
+            mid = (low & high) + (low ^ high) / 2; // rounds down


Why we say that the simple (low + high) / 2; doesn't round down?

yes, it rounds down to the closest integer, the other way it rounds down towards zero, truncate (I should have specified that).

Didn't understand what you mean. Rounds towards zero?

Maybe with some concrete examples for both ways I can understand what you mean.

Suggested change

mid = (low & high) + (low ^ high) / 2; // rounds down

mid = (low & high) + (low ^ high) / 2; // rounds down to zero, truncate

Yeah ok, that's a complete different explanation from what you mentioned above.
It doesn't have anything to do with one "rounding down to the closest integer" vs the other "rounding down towards zero, truncate" (as they both mean the same exact same thing).
They both do the exact same thing, rounds down to the closest integer or rounds down towards zero.
The only difference is one avoids a possible overflow. So let's try to understand the reasons before doing a change and explain it.

In this specific case, the overflow won't happen in practice, so maybe it is just better to keep the simpler form. To be honest I would need to start digging in order to understand why this math does the same thing (just glancing I don't get it). Maybe as it is part of a known library we can just leave it.

testing on remix I got both implementation rounding towards zero or truncate and checking solc docs that's seems the case, maybe the issue I was having here was overflow of some sort.

how do you have an overflow summing two block numbers?

so, testing again against the blocks above, where I noticed the strange pattern (pass, fail, stuck) it seems to pass now with both implementations, so the issue was probably the comparison and the mid bump off by one, but anyway, I think the proposed implementation looks better for general purposes, in case the binary search pattern will be reused, and I agree that a rational would have been helpful there but was driven mostly by reusing a consolidated library pattern, adjusting it to our needs and cover the test scenarios where the previous implementation was having issues and sharing for reviews and discussion.

gbalabasquer · 2023-02-14T16:13:58Z

src/DssSpell.t.base.sol

-        spellValues.deployed_spell_created = spellValues.deployed_spell != address(0) ? spellValues.deployed_spell_created : block.timestamp;
+        if (spellValues.deployed_spell != address(0)) {
+            spell = DssSpell(spellValues.deployed_spell);
+            if (spell.eta() != 0) {


Hmmm isn't there an issue here? We are removing the created timestamp as we assume it will be always block.timestamp (as we are rolling to the deployment block). However if the spell was deployed but not scheduled, this condition won't be true, then there won't be a roll to the deployment block, meaning that we will be assuming an incorrect timestamp as the deployment of the spell.
The immediate solution is removing this if rolling always that exists a deployed spell.

This was added so that during the time after deployment before scheduling, spell crafters and reviewers do not need a local archive node. Once the spell is scheduled the local tests without an archive would fail (already scheduled), with this they will fail because forge can't rollFork. But I see your point about the later timestamp tests. Maybe we need to keep that piece in the config?

Right, if we want to keep this testing without archive, then we will need to do so, put back the timestamp in the config file. But doing that, I almost feel that all the binary search is less worth it and we should just keep the timestamp and block in the config.

maybe it would be worth it to see if the deploy script could automatically update with the spell address, timestamp, block number and then keep those values hardcoded?

Hmmm you mean that the script reads the spell address from the config and looks for the timestamp and block and then saves it in the same config file?

An alternative solution is always requesting an archive node, which IMO is a super expensive price to pay just for avoiding to complete two values in the config.

I just tested that case and I confirm testGeneral Fails testing on block after the deployment one (via make test block=8451435)

Fast nodes do have some archive state, but only the most recent 128 blocks... this won't be suitable for our purposes even if we cache the block number. Will need some further thought, maybe the solution, instead of testing the exact expiration time, would be to modify the test to ensure that the expiration is lteq now + expiration_threshold instead of eq deployed time + expiration threshold

Proposed fix here #172

Hmmm aren't we being less specific with that test though? I mean we are putting a weaker check.
On the other hand I was more worried about all the tests that check the specific cast time and so, aren't those failing as well?

* add block start point caching * check low block first * don't write or roll a second time if low is correct' * move file var to function * forgot memory

DaiFoundation-DevOps · 2023-07-06T15:59:41Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 3 committers have signed the CLA.

❌ naszam
❌ brianmcmichael
❌ iamchrissmith
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

Utilizes rollfork to obsolete deploy-info script

225260b

brianmcmichael requested review from iamchrissmith, gbalabasquer and naszam February 11, 2023 01:13

brianmcmichael added 2 commits February 10, 2023 22:55

no return

778a72d

return on 0 address

82d733c

naszam reviewed Feb 13, 2023

View reviewed changes

src/DssSpell.t.base.sol Outdated Show resolved Hide resolved

naszam mentioned this pull request Feb 13, 2023

Fix binary search #167

Merged

Fix binary search (#167)

2d96dfe

naszam previously approved these changes Feb 13, 2023

View reviewed changes

naszam mentioned this pull request Feb 13, 2023

Cleanups & Refinements makerdao/pe-checklists#10

Merged

iamchrissmith reviewed Feb 13, 2023

View reviewed changes

gbalabasquer reviewed Feb 14, 2023

View reviewed changes

naszam mentioned this pull request Feb 20, 2023

Improve get-deploy-info.sh #170

Closed

Infoless deploy test caching (#168)

11cc64f

* add block start point caching * check low block first * don't write or roll a second time if low is correct' * move file var to function * forgot memory

iamchrissmith dismissed naszam’s stale review via 11cc64f February 21, 2023 00:28

naszam mentioned this pull request Feb 24, 2023

Improve deploy.sh #171

Merged

brianmcmichael mentioned this pull request Feb 24, 2023

Infoless deploy test timestamp test #172

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Utilizes rollfork to obsolete deploy-info script #166

Utilizes rollfork to obsolete deploy-info script #166

brianmcmichael commented Feb 11, 2023

naszam left a comment

iamchrissmith Feb 13, 2023

iamchrissmith Feb 13, 2023

gbalabasquer Feb 14, 2023

iamchrissmith Feb 14, 2023

brianmcmichael Feb 20, 2023

iamchrissmith Feb 13, 2023

brianmcmichael Feb 20, 2023

iamchrissmith Feb 13, 2023

gbalabasquer Feb 14, 2023

brianmcmichael Feb 20, 2023

brianmcmichael Feb 23, 2023

gbalabasquer Feb 14, 2023

naszam Feb 14, 2023

gbalabasquer Feb 14, 2023

gbalabasquer Feb 14, 2023

naszam Feb 14, 2023

naszam Feb 14, 2023

gbalabasquer Feb 14, 2023

naszam Feb 14, 2023 •

edited

Loading

gbalabasquer Feb 14, 2023

naszam Feb 14, 2023 •

edited

Loading

gbalabasquer Feb 14, 2023

iamchrissmith Feb 14, 2023

gbalabasquer Feb 15, 2023

iamchrissmith Feb 15, 2023

gbalabasquer Feb 15, 2023

gbalabasquer Feb 24, 2023

naszam Feb 24, 2023

brianmcmichael Feb 24, 2023 •

edited

Loading

brianmcmichael Feb 24, 2023 •

edited

Loading

gbalabasquer Feb 24, 2023

DaiFoundation-DevOps commented Jul 6, 2023 •

edited

Loading

	uint256 low = 5273074; // MCD_VAT Deployed Aug-06-2021 04:26:38 PM +UTC
	uint256 min = 5273074; // MCD_VAT Deployed Aug-06-2021 04:26:38 PM +UTC

-                // we want to roll our fork to the block where it was deployed
-                // this means the test suite will continue to accurately pass/fail
-                // even if mainnet has already scheduled/cast the spell
+                // we want to roll our fork to a block after it was deployed and
+                // before it was scheduled this means the test suite will continue
+                // to accurately pass/fail even if goerli has already
+                // scheduled/cast the spell

	mid = (low & high) + (low ^ high) / 2; // rounds down
	mid = (low & high) + (low ^ high) / 2; // rounds down to zero, truncate

Utilizes rollfork to obsolete deploy-info script #166

Are you sure you want to change the base?

Utilizes rollfork to obsolete deploy-info script #166

Conversation

brianmcmichael commented Feb 11, 2023

Description

naszam 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

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

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

Choose a reason for hiding this comment

naszam Feb 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naszam Feb 14, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianmcmichael Feb 24, 2023 • edited Loading

Choose a reason for hiding this comment

brianmcmichael Feb 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaiFoundation-DevOps commented Jul 6, 2023 • edited Loading

naszam Feb 14, 2023 •

edited

Loading

naszam Feb 14, 2023 •

edited

Loading

brianmcmichael Feb 24, 2023 •

edited

Loading

brianmcmichael Feb 24, 2023 •

edited

Loading

DaiFoundation-DevOps commented Jul 6, 2023 •

edited

Loading