Skip to content

Commit

Permalink
refactor: expand testing chainlog integrity capabilities
Browse files Browse the repository at this point in the history
  • Loading branch information
@amusingaxl
amusingaxl committed Jan 15, 2024
1 parent ee11353 commit 9a4dba6
Show file tree
Hide file tree
Showing 2 changed files with 148 additions and 62 deletions.
192 changes: 144 additions & 48 deletions src/DssSpell.t.base.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1808,74 +1808,170 @@ contract DssSpellTestBase is Config, DssTest {
assertEq(actualHash, expectedHash);
}

// Validate addresses in test harness match chainlog
function _testChainlogValues() internal {
struct ChainlogCache {
bytes32 versionHash;
bytes32 contentHash;
uint256 count;
bytes32[] keys;
address[] values;
}

/**
* @dev Checks the integrity of the chainlog.
* This test case is able to catch the following spell issues:
* 1. Modifications without version bump:
* a. Removing a key.
* b. Updating a key.
* c. Adding a key.
* d. Removing a key and adding it back (this can change the order of the list).
* 2. Version bumping without modifications.
* 3. Dangling wards on new or updated keys.
*
* When adding or updating a key, the test will automatically check for dangling wards if applicable.
* Notice that when a key is removed, if it is not the last one, there is a side-effect of moving
* the last key to the position of the removed one (well-known Solidity iterability pattern).
* This will generate a false-positive that will cause the test to re-check wards for the moved key.
*/
function _testChainlogIntegrity() internal {
ChainlogCache memory cacheBefore = ChainlogCache({
count: chainLog.count(),
keys: chainLog.list(),
versionHash: keccak256(abi.encodePacked("")),
contentHash: keccak256(abi.encode(new bytes32[](0), new address[](0))),
values: new address[](0)
});

cacheBefore.values = new address[](cacheBefore.count);
for(uint256 i = 0; i < cacheBefore.count; i++) {
cacheBefore.values[i] = chainLog.getAddress(cacheBefore.keys[i]);
}

cacheBefore.versionHash = keccak256(abi.encodePacked(chainLog.version()));
// Using `abi.encode` to prevent ambiguous encoding
cacheBefore.contentHash = keccak256(abi.encode(cacheBefore.keys, cacheBefore.values));

//////////////////////////////////////////

_vote(address(spell));
_scheduleWaitAndCast(address(spell));
assertTrue(spell.done());

bytes32[] memory keys = chainLog.list();
for(uint256 i = 0; i < keys.length; i++) {
assertEq(
chainLog.getAddress(keys[i]),
addr.addr(keys[i]),
_concat("TestError/chainlog-vs-harness-key-mismatch: ", keys[i])
);
}
//////////////////////////////////////////

assertEq(chainLog.version(), afterSpell.chainlog_version, "TestError/chainlog-version-mismatch");
}
ChainlogCache memory cacheAfter = ChainlogCache({
count: chainLog.count(),
keys: chainLog.list(),
versionHash: keccak256(abi.encodePacked("")),
contentHash: keccak256(abi.encode(new bytes32[](0), new address[](0))),
values: new address[](0)
});

// Ensure version is updated if chainlog changes
function _testChainlogVersionBump() internal {
string memory pversion = chainLog.version();
bytes32[] memory pkeys = chainLog.list();
uint256 pcount = pkeys.length;
address[] memory pvalues = new address[](pcount);
cacheAfter.values = new address[](cacheAfter.count);
for(uint256 i = 0; i < cacheAfter.count; i++) {
cacheAfter.values[i] = chainLog.getAddress(cacheAfter.keys[i]);
}

for(uint256 i = 0; i < pcount; i++) {
pvalues[i] = chainLog.getAddress(pkeys[i]);
cacheAfter.versionHash = keccak256(abi.encodePacked(chainLog.version()));
// Using `abi.encode` to prevent ambiguous encoding
cacheAfter.contentHash = keccak256(abi.encode(cacheAfter.keys, cacheAfter.values));

//////////////////////////////////////////

// If the version is the same, the content should not have changed
if (cacheAfter.versionHash == cacheBefore.versionHash) {
assertEq(cacheBefore.count, cacheAfter.count, "TestError/chainlog-version-not-updated-length-change");

// Add explicit check otherwise this would fail with an array-out-of-bounds error,
// since Foundry does not halt the execution when an assertion fails.
if (cacheBefore.count == cacheAfter.count) {
// Fail if the chainlog is the same size, but order of keys changed
for (uint256 i = 0; i < cacheAfter.count; i++) {
assertEq(
cacheBefore.values[i],
cacheAfter.values[i],
_concat(
"TestError/chainlog-version-not-updated-value-change: ",
_concat(
_concat("+ ", cacheAfter.keys[i]),
_concat(" | - ", cacheBefore.keys[i])
)
)
);
}
}
} else {
// If the version changed, the content should have changed
assertTrue(cacheAfter.contentHash != cacheBefore.contentHash, "TestError/chainlog-version-updated-no-content-change");
}

_vote(address(spell));
_scheduleWaitAndCast(address(spell));
assertTrue(spell.done());
// If the content has changed, we look into the diff
if (cacheAfter.contentHash != cacheBefore.contentHash) {
uint256 diffCount;
// Iteration must stop at the shorter array length
uint256 maxIters = cacheAfter.count > cacheBefore.count ? cacheBefore.count : cacheAfter.count;

// Look for changes in existing keys
for (uint256 i = 0; i < maxIters; i++) {
if (cacheAfter.keys[i] != cacheBefore.keys[i]) {
// Change in order
diffCount += 1;
} else if (cacheAfter.values[i] != cacheBefore.values[i]) {
// Change in value
diffCount += 1;
}
}

if (keccak256(abi.encodePacked(pversion)) == keccak256(abi.encodePacked(chainLog.version()))) {
bytes32[] memory keys = chainLog.list();
uint256 count = keys.length;
address[] memory values = new address[](count);

// Fail if the version is not updated and the chainlog count has changed
assertEq(count, pcount, "TestError/chainlog-version-not-updated-count-change");

for(uint256 i = 0; i < count; i++) {
values[i] = chainLog.getAddress(keys[i]);
// Fail if the chainlog is the same size, but any address or their order changed.
assertEq(
values[i],
pvalues[i],
_concat("TestError/chainlog-version-not-updated-address-change: ", keys[i])
);
// Account for new keys
// Notice: we don't care about removed keys
if (cacheAfter.count > cacheBefore.count) {
diffCount += (cacheAfter.count - cacheBefore.count);
}

////////////////////////////////////////

bytes32[] memory diffKeys = new bytes32[](diffCount);
uint256 j = 0;

for (uint256 i = 0; i < maxIters; i++) {
if (cacheAfter.keys[i] != cacheBefore.keys[i]) {
// Mark keys whose order has changed
diffKeys[j++] = cacheAfter.keys[i];
} else if (cacheAfter.values[i] != cacheBefore.values[i]) {
// Mark changed values
diffKeys[j++] = cacheAfter.keys[i];
}
}

// Mark new keys
if (cacheAfter.count > cacheBefore.count) {
for (uint256 i = cacheBefore.count; i < cacheAfter.count; i++) {
diffKeys[j++] = cacheAfter.keys[i];
}
}

for (uint256 i = 0; i < diffKeys.length; i++) {
_checkAuth(diffKeys[i]);
}
}
}

function _testNewOrUpdatedChainlogValues(bytes32[] memory keys) internal {
// `setAddress` must have being called `keys.length` times.
vm.expectCall(
address(chainLog),
abi.encodeWithSelector(chainLog.setAddress.selector),
uint64(keys.length)
);

// Validate addresses in test harness match chainlog
function _testChainlogValues() internal {
_vote(address(spell));
_scheduleWaitAndCast(address(spell));
assertTrue(spell.done());

bytes32[] memory keys = chainLog.list();
for (uint256 i = 0; i < keys.length; i++) {
_checkAuth(keys[i]);
assertEq(
chainLog.getAddress(keys[i]),
addr.addr(keys[i]),
_concat("TestError/chainlog-vs-harness-key-mismatch: ", keys[i])
);
}

assertEq(chainLog.version(), afterSpell.chainlog_version, "TestError/chainlog-version-mismatch");
}

function _checkCropCRVLPIntegration(
Expand Down
18 changes: 4 additions & 14 deletions src/DssSpell.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,12 +99,12 @@ contract DssSpellTest is DssSpellTestBase {
_testBytecodeMatches();
}

function testChainlogValues() public {
_testChainlogValues();
function testChainlogIntegrity() public {
_testChainlogIntegrity();
}

function testChainlogVersionBump() public {
_testChainlogVersionBump();
function testChainlogValues() public {
_testChainlogValues();
}

// Leave this test public (for now) as this is acting like a config test
Expand Down Expand Up @@ -274,16 +274,6 @@ contract DssSpellTest is DssSpellTestBase {
assertTrue(lerp.done());
}

function testNewOrUpdatedChainlogValues() private { // make private to disable
bytes32[] memory keys = new bytes32[](4);
keys[0] = "MCD_PSM_GUSD_A_JAR";
keys[1] = "MCD_PSM_GUSD_A_INPUT_CONDUIT_JAR";
keys[2] = "MCD_PSM_PAX_A_JAR";
keys[3] = "MCD_PSM_PAX_A_INPUT_CONDUIT_JAR";

_testNewOrUpdatedChainlogValues(keys);
}

function testNewIlkRegistryValues() private { // make private to disable
_vote(address(spell));
_scheduleWaitAndCast(address(spell));
Expand Down

0 comments on commit 9a4dba6

Please sign in to comment.