Skip to content

Commit

Permalink
fix: update datatype
Browse files Browse the repository at this point in the history
  • Loading branch information
excaliborr committed Nov 21, 2023
1 parent 8e55d38 commit c962c72
Show file tree
Hide file tree
Showing 5 changed files with 36 additions and 38 deletions.
9 changes: 4 additions & 5 deletions solidity/contracts/StorageMirror.sol
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,11 @@ contract StorageMirror is IStorageMirror {
/**
* @notice Updates a safe's settings hash
* @dev The safe should always be msg.sender
* @param _safeSettings The settings we are going to update to
* @param _hashedSafeSettings The hashed settings we are going to update to
*/
function update(SafeSettings memory _safeSettings) external {
bytes32 _settingsHash = keccak256(abi.encode(_safeSettings));
latestSettingsHash[msg.sender] = _settingsHash;
function update(bytes32 _hashedSafeSettings) external {
latestSettingsHash[msg.sender] = _hashedSafeSettings;

emit SettingsUpdated(msg.sender, _settingsHash, _safeSettings);
emit SettingsUpdated(msg.sender, _hashedSafeSettings);
}
}
20 changes: 11 additions & 9 deletions solidity/contracts/UpdateStorageMirrorGuard.sol
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,12 @@ contract UpdateStorageMirrorGuard is BaseGuard {
/**
* @notice A boolean that returns true if a tx is changing the safe's settings
*/
bool public didSettingsChange;
mapping(address => bool) public didSettingsChange;

/**
* @notice The hash of the new settings
*/
bytes32 public settingsHash;
mapping(address => bytes32) public settingsHash;

constructor(IGuardCallbackModule _guardCallbackModule) {
GUARD_CALLBACK_MODULE = _guardCallbackModule;
Expand All @@ -52,14 +52,16 @@ contract UpdateStorageMirrorGuard is BaseGuard {
bytes memory _signatures,
address _msgSender
) external {
didSettingsChange = true;
didSettingsChange[msg.sender] = true;
// TODO: change these data with the decoded ones
address[] memory _owners = new address[](1);
IStorageMirror.SafeSettings memory _safeSettings = IStorageMirror.SafeSettings({owners: _owners, threshold: 1});

settingsHash = keccak256(abi.encode(_safeSettings));
bytes32 _settingsHash = keccak256(abi.encode(_safeSettings));
settingsHash[msg.sender] = _settingsHash;
didSettingsChange[msg.sender] = true;

emit SettingsChanged(msg.sender, settingsHash, _safeSettings);
emit SettingsChanged(msg.sender, _settingsHash, _safeSettings);
}

/**
Expand All @@ -68,10 +70,10 @@ contract UpdateStorageMirrorGuard is BaseGuard {
* @dev The msg.sender should be the safe
*/
function checkAfterExecution(bytes32 _txHash, bool _success) external {
if (didSettingsChange && _success) {
GUARD_CALLBACK_MODULE.saveUpdatedSettings(msg.sender, settingsHash);
didSettingsChange = false;
settingsHash = keccak256(abi.encodePacked(''));
if (didSettingsChange[msg.sender] && _success) {
// NOTE: No need to reset settings as this function will only be called when the settings change
GUARD_CALLBACK_MODULE.saveUpdatedSettings(msg.sender, settingsHash[msg.sender]);
didSettingsChange[msg.sender] = false;
}
}
}
7 changes: 3 additions & 4 deletions solidity/interfaces/IStorageMirror.sol
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,8 @@ interface IStorageMirror {
*
* @param _safe The address of the safe
* @param _settingsHash The hash of the settings
* @param _safeSettings The plaintext of the settings
*/
event SettingsUpdated(address indexed _safe, bytes32 indexed _settingsHash, SafeSettings _safeSettings);
event SettingsUpdated(address indexed _safe, bytes32 indexed _settingsHash);

/*///////////////////////////////////////////////////////////////
STRUCTS
Expand Down Expand Up @@ -42,7 +41,7 @@ interface IStorageMirror {
/**
* @notice Updates a safe's settings hash
* @dev The safe should always be msg.sender
* @param _safeSettings The settings we are going to update to
* @param _hashedSafeSettings The hashed settings we are going to update to
*/
function update(SafeSettings memory _safeSettings) external;
function update(bytes32 _hashedSafeSettings) external;
}
13 changes: 6 additions & 7 deletions solidity/test/unit/StorageMirror.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,7 @@ import {StorageMirror} from 'contracts/StorageMirror.sol';
import {IStorageMirror} from 'interfaces/IStorageMirror.sol';

abstract contract Base is Test {
event SettingsUpdated(
address indexed _safe, bytes32 indexed _settingsHash, IStorageMirror.SafeSettings _safeSettings
);
event SettingsUpdated(address indexed _safe, bytes32 indexed _settingsHash);

address public safe;
StorageMirror public storageMirror;
Expand All @@ -27,10 +25,11 @@ contract UnitStorageMirror is Base {
IStorageMirror.SafeSettings memory _safeSettings =
IStorageMirror.SafeSettings({owners: _owners, threshold: _threshold});

bytes32 _settingsHash = keccak256(abi.encode(_safeSettings));

vm.prank(safe);
storageMirror.update(_safeSettings);
storageMirror.update(_settingsHash);

bytes32 _settingsHash = keccak256(abi.encode(_safeSettings));
bytes32 _savedHash = storageMirror.latestSettingsHash(safe);

assertEq(_settingsHash, _savedHash, 'Settings hash should be saved');
Expand All @@ -47,7 +46,7 @@ contract UnitStorageMirror is Base {

vm.prank(safe);
vm.expectEmit(true, true, true, true);
emit SettingsUpdated(safe, _expectedSettingsHash, _safeSettings);
storageMirror.update(_safeSettings);
emit SettingsUpdated(safe, _expectedSettingsHash);
storageMirror.update(keccak256(abi.encode(_safeSettings)));
}
}
25 changes: 12 additions & 13 deletions solidity/test/unit/UpdateStorageMirrorGuard.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -27,25 +27,25 @@ abstract contract Base is Test {

contract UnitUpdateStorageMirrorGuard is Base {
function testCheckTransaction() public {
assertFalse(updateStorageMirrorGuard.didSettingsChange());
assertEq(updateStorageMirrorGuard.settingsHash(), bytes32(''));
assertFalse(updateStorageMirrorGuard.didSettingsChange(safe));
assertEq(updateStorageMirrorGuard.settingsHash(safe), bytes32(''));

vm.expectEmit(true, true, true, true);
emit SettingsChanged(safe, settingsHash, safeSettings);
vm.prank(safe);
updateStorageMirrorGuard.checkTransaction(
address(0), 0, '', Enum.Operation.Call, 0, 0, 0, address(0), payable(0), '', address(0)
address(0), 0, '', Enum.Operation.Call, 0, 0, 0, address(0), payable(0), '', safe
);

assertTrue(updateStorageMirrorGuard.didSettingsChange());
assertEq(updateStorageMirrorGuard.settingsHash(), settingsHash, 'Settings hash should be stored');
assertTrue(updateStorageMirrorGuard.didSettingsChange(safe));
assertEq(updateStorageMirrorGuard.settingsHash(safe), settingsHash, 'Settings hash should be stored');
}

function testCheckAfterExecution(bytes32 _txHash) public {
// Call checkTransaction to change didSettingsChange to true
vm.prank(safe);
updateStorageMirrorGuard.checkTransaction(
address(0), 0, '', Enum.Operation.Call, 0, 0, 0, address(0), payable(0), '', address(0)
address(0), 0, '', Enum.Operation.Call, 0, 0, 0, address(0), payable(0), '', safe
);

vm.mockCall(
Expand All @@ -59,30 +59,29 @@ contract UnitUpdateStorageMirrorGuard is Base {
vm.prank(safe);
updateStorageMirrorGuard.checkAfterExecution(_txHash, true);

assertFalse(updateStorageMirrorGuard.didSettingsChange());
assertEq(updateStorageMirrorGuard.settingsHash(), keccak256(abi.encodePacked('')), 'Settings hash should reset');
assertFalse(updateStorageMirrorGuard.didSettingsChange(safe));
}

function testCheckAfterExecutionNoSettingsChange(bytes32 _txHash) public {
vm.prank(safe);
updateStorageMirrorGuard.checkAfterExecution(_txHash, true);

assertFalse(updateStorageMirrorGuard.didSettingsChange());
assertEq(updateStorageMirrorGuard.settingsHash(), bytes32(''), 'Settings hash should stay empty');
assertFalse(updateStorageMirrorGuard.didSettingsChange(safe));
assertEq(updateStorageMirrorGuard.settingsHash(safe), bytes32(''), 'Settings hash should stay empty');
}

function testCheckAfterExecutionTxFailed(bytes32 _txHash) public {
// Call checkTransaction to change didSettingsChange to true
vm.prank(safe);
updateStorageMirrorGuard.checkTransaction(
address(0), 0, '', Enum.Operation.Call, 0, 0, 0, address(0), payable(0), '', address(0)
address(0), 0, '', Enum.Operation.Call, 0, 0, 0, address(0), payable(0), '', safe
);

vm.prank(safe);
updateStorageMirrorGuard.checkAfterExecution(_txHash, false);

// Should be true since the tx failed to execute and thus didnt make it to reset
assertTrue(updateStorageMirrorGuard.didSettingsChange());
assertEq(updateStorageMirrorGuard.settingsHash(), settingsHash, 'Settings hash should stay the same');
assertTrue(updateStorageMirrorGuard.didSettingsChange(safe));
assertEq(updateStorageMirrorGuard.settingsHash(safe), settingsHash, 'Settings hash should stay the same');
}
}

0 comments on commit c962c72

Please sign in to comment.