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 all 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
3 changes: 2 additions & 1 deletion foundry.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ out = 'out'
libs = ['lib']
fs_permissions = [
{ access = "read", path = "./lib/dss-test/script/input/"},
{ access = "read", path = "./out/ArbitrumDomain.sol/ArbSysOverride.json"}
{ access = "read", path = "./out/ArbitrumDomain.sol/ArbSysOverride.json"},
{ access = "read-write", path = "./spellblock.txt"}
]

[rpc_endpoints]
Expand Down
1 change: 1 addition & 0 deletions spellblock.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
8451434
70 changes: 51 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,24 +266,60 @@ 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;
}

string memory spellBlockPath = "./spellblock.txt";

uint256 low = vm.parseUint(vm.readLine(spellBlockPath));
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;

vm.rollFork(low);
if (!_isContractDeployed(_spell)) {
while (low < high) {
mid = (low & high) + (low ^ high) / 2; // rounds down
vm.rollFork(mid);
if (_isContractDeployed(_spell)) {
high = mid;
} else {
low = mid + 1;
}
}
vm.writeFile(spellBlockPath, vm.toString(low));
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 {
if (chief.hat() != spell_) {
_giveTokens(address(gov), 999999999999 ether);
Expand Down Expand Up @@ -1446,11 +1482,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