Skip to content
This repository has been archived by the owner on Apr 22, 2024. It is now read-only.

Utilizes rollfork to obsolete deploy-info script #166

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Utilizes rollfork to obsolete deploy-info script #166

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
225260b
Utilizes rollfork to obsolete deploy-info script
brianmcmichael Feb 11, 2023
778a72d
no return
brianmcmichael Feb 11, 2023
82d733c
return on 0 address
brianmcmichael Feb 11, 2023
2d96dfe
Fix binary search (#167)
naszam Feb 13, 2023
11cc64f
Infoless deploy test caching (#168)
iamchrissmith Feb 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 46 additions & 19 deletions src/DssSpell.t.base.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,22 +266,53 @@ contract DssSpellTestBase is Config, DssTest {
function setUp() public {
setValues(address(chief));

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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@brianmcmichael brianmcmichael Feb 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@brianmcmichael brianmcmichael Feb 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Proposed fix here #172

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

// if we have a scheduled spell in the config
// 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
Comment on lines +273 to +275
Copy link
Contributor

Choose a reason for hiding this comment

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

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

vm.makePersistent(address(this));
vm.makePersistent(address(rates));
vm.makePersistent(address(addr));
vm.makePersistent(address(deployers));
_rollToDeployedSpellBlock(address(spell));
}
} else {
spell = new DssSpell();
}

_castPreviousSpell();
spell = spellValues.deployed_spell != address(0) ?
DssSpell(spellValues.deployed_spell) : new DssSpell();

if (spellValues.deployed_spell_block != 0 && spell.eta() != 0) {
// if we have a deployed spell in the config
// 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
vm.makePersistent(address(this));
vm.makePersistent(address(rates));
vm.makePersistent(address(addr));
vm.makePersistent(address(deployers));
vm.rollFork(spellValues.deployed_spell_block);
}

function _rollToDeployedSpellBlock(address _spell) internal {
if (_spell == address(0)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reusability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

vm.rollFork(block.number);
return;
}

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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I created this to address this issue: #168

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

uint256 high = block.number;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
uint256 high = block.number;
uint256 max = block.number;

uint256 mid;

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

see here

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@naszam naszam Feb 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

how do you have an overflow summing two block numbers?

Copy link
Contributor

@naszam naszam Feb 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

vm.rollFork(mid);
if (_isContractDeployed(_spell)) {
high = mid;
} else {
low = mid + 1;
}
}

vm.rollFork(low);
}

function _isContractDeployed(address _spell) internal view returns (bool) {
uint256 size;
assembly { size := extcodesize(_spell) }
return size > 0;
}

function _vote(address spell_) internal {
Expand Down Expand Up @@ -1446,11 +1477,7 @@ contract DssSpellTestBase is Config, DssTest {
assertEq(_stringToBytes32(spell.description()),
_stringToBytes32(description), "TestError/spell-description");

if(address(spell) != address(spellValues.deployed_spell)) {
assertEq(spell.expiration(), block.timestamp + spellValues.expiration_threshold, "TestError/spell-expiration");
} else {
assertEq(spell.expiration(), spellValues.deployed_spell_created + spellValues.expiration_threshold, "TestError/spell-expiration");
}
assertEq(spell.expiration(), block.timestamp + spellValues.expiration_threshold, "TestError/spell-expiration");

assertTrue(spell.officeHours() == spellValues.office_hours_enabled, "TestError/spell-office-hours");

Expand Down
4 changes: 0 additions & 4 deletions src/test/config.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@ contract Config {

struct SpellValues {
address deployed_spell;
uint256 deployed_spell_created;
uint256 deployed_spell_block;
address previous_spell;
bool office_hours_enabled;
uint256 expiration_threshold;
Expand Down Expand Up @@ -96,8 +94,6 @@ contract Config {
//
spellValues = SpellValues({
deployed_spell: address(0x89625bb73bA7Dccde985F690CEa2659525AB5b15), // populate with deployed spell if deployed
deployed_spell_created: 1675789032, // use `./scripts/get-created-timestamp.sh <deployment-tx>`
deployed_spell_block: 8451434, // populate with the block where the spell was deployed
previous_spell: address(0), // supply if there is a need to test prior to its cast() function being called on-chain.
office_hours_enabled: false, // true if officehours is expected to be enabled in the spell
expiration_threshold: 30 days // Amount of time before spell expires
Expand Down