Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CI, Contracts: Add forge fmt --check to CI, run forge fmt #192

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 4 additions & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ jobs:
with:
version: nightly

- name: Run Linter
run: forge fmt --check

- name: Run tests
run: forge test --force -vvv

Expand All @@ -27,4 +30,4 @@ jobs:
- name: Fork tests (polygon)
env:
FORK_URL: ${{ secrets.POLYGON_FORK_URL }}
run: forge test --match-contract FirmFactoryIntegrationTest --fork-url "$FORK_URL" -vvvv
run: forge test --match-contract FirmFactoryIntegrationTest --fork-url "$FORK_URL" -vvvv
3 changes: 2 additions & 1 deletion src/bases/SafeAware.sol
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ abstract contract SafeAware {
* @dev Modifier to be used by derived contracts to limit access control to priviledged
* functions so they can only be called by the Safe
*/

modifier onlySafe() {
if (_msgSender() != address(safe())) {
revert UnauthorizedNotSafe();
Expand All @@ -56,5 +57,5 @@ abstract contract SafeAware {
_;
}

function _msgSender() internal view virtual returns (address sender);
function _msgSender() internal view virtual returns (address sender);
}
17 changes: 11 additions & 6 deletions src/bases/SemaphoreAuth.sol
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {SafeAware} from "./SafeAware.sol";
import {ISemaphore} from "../semaphore/interfaces/ISemaphore.sol";

ISemaphore constant NO_SEMAPHORE = ISemaphore(address(0));

abstract contract SemaphoreAuth is SafeAware {
// SEMAPHORE_SLOT = keccak256("firm.semaphoreauth.semaphore") - 1
bytes32 internal constant SEMAPHORE_SLOT = 0x3e4ab72c2ecd29625ea852b1de3f6381681f0f5fb2b0bab359fabdd96dbc1b94;
Expand All @@ -31,17 +32,21 @@ abstract contract SemaphoreAuth is SafeAware {

function _semaphoreCheckCall(address target, uint256 value, bytes memory data, bool isDelegateCall) internal view {
ISemaphore semaphore_ = semaphore();
if (semaphore_ != NO_SEMAPHORE &&
!semaphore_.canPerform(address(this), target, value, data, isDelegateCall)
) {
if (semaphore_ != NO_SEMAPHORE && !semaphore_.canPerform(address(this), target, value, data, isDelegateCall)) {
revert ISemaphore.SemaphoreDisallowed();
}
}

function _semaphoreCheckCalls(address[] memory targets, uint256[] memory values, bytes[] memory datas, bool isDelegateCall) internal view {
function _semaphoreCheckCalls(
address[] memory targets,
uint256[] memory values,
bytes[] memory datas,
bool isDelegateCall
) internal view {
ISemaphore semaphore_ = semaphore();
if (semaphore_ != NO_SEMAPHORE &&
!semaphore_.canPerformMany(address(this), targets, values, datas, isDelegateCall)
if (
semaphore_ != NO_SEMAPHORE
&& !semaphore_.canPerformMany(address(this), targets, values, datas, isDelegateCall)
) {
revert ISemaphore.SemaphoreDisallowed();
}
Expand Down
20 changes: 14 additions & 6 deletions src/bases/test/SemaphoreAuth.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,11 @@ contract SemaphoreAuthTest is FirmTest {
function setUp() public {
safe = new SafeStub();
semaphore = new SemaphoreStub();
semaphoreAuth = SemaphoreAuthMock(createProxy(new SemaphoreAuthMock(), abi.encodeCall(SemaphoreAuthMock.initialize, (ISafe(payable(safe)), semaphore))));
semaphoreAuth = SemaphoreAuthMock(
createProxy(
new SemaphoreAuthMock(), abi.encodeCall(SemaphoreAuthMock.initialize, (ISafe(payable(safe)), semaphore))
)
);
}

function testInitialState() public {
Expand Down Expand Up @@ -80,7 +84,8 @@ contract SemaphoreAuthTest is FirmTest {
semaphoreAuth.semaphoreCheckCalls(targets, values, datas, false);

// We filter some target (the one causing the revert) out
(address[] memory filteredTargets, uint256[] memory filteredValues, bytes[] memory filteredDatas) = semaphoreAuth.filterCallsToTarget(someTarget, targets, values, datas);
(address[] memory filteredTargets, uint256[] memory filteredValues, bytes[] memory filteredDatas) =
semaphoreAuth.filterCallsToTarget(someTarget, targets, values, datas);

// And now it doesn't revert
semaphoreAuth.semaphoreCheckCalls(filteredTargets, filteredValues, filteredDatas, false);
Expand All @@ -105,7 +110,8 @@ contract SemaphoreAuthTest is FirmTest {
datas[0] = bytes("data1");
datas[1] = bytes("data2");

(address[] memory filteredTargets, uint256[] memory filteredValues, bytes[] memory filteredDatas) = semaphoreAuth.filterCallsToTarget(someTarget, targets, values, datas);
(address[] memory filteredTargets, uint256[] memory filteredValues, bytes[] memory filteredDatas) =
semaphoreAuth.filterCallsToTarget(someTarget, targets, values, datas);

assertEq(filteredTargets.length, 1);
assertEq(filteredTargets[0], someOtherTarget);
Expand All @@ -130,7 +136,8 @@ contract SemaphoreAuthTest is FirmTest {
datas[0] = bytes("data1");
datas[1] = bytes("data2");

(address[] memory filteredTargets, uint256[] memory filteredValues, bytes[] memory filteredDatas) = semaphoreAuth.filterCallsToTarget(account("Another target"), targets, values, datas);
(address[] memory filteredTargets, uint256[] memory filteredValues, bytes[] memory filteredDatas) =
semaphoreAuth.filterCallsToTarget(account("Another target"), targets, values, datas);

assertEq(filteredTargets.length, 2);
assertEq(filteredTargets[0], someTarget);
Expand Down Expand Up @@ -158,10 +165,11 @@ contract SemaphoreAuthTest is FirmTest {
datas[0] = bytes("data1");
datas[1] = bytes("data2");

(address[] memory filteredTargets, uint256[] memory filteredValues, bytes[] memory filteredDatas) = semaphoreAuth.filterCallsToTarget(someTarget, targets, values, datas);
(address[] memory filteredTargets, uint256[] memory filteredValues, bytes[] memory filteredDatas) =
semaphoreAuth.filterCallsToTarget(someTarget, targets, values, datas);

assertEq(filteredTargets.length, 0);
assertEq(filteredValues.length, 0);
assertEq(filteredDatas.length, 0);
}
}
}
2 changes: 1 addition & 1 deletion src/bases/test/lib/RolesAuthFlags.sol
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,4 @@ import {ROLES_FLAG_TYPE, AddressUint8FlagsLib} from "../../RolesAuth.sol";

function roleFlag(uint8 role) pure returns (address) {
return AddressUint8FlagsLib.toFlag(role, ROLES_FLAG_TYPE);
}
}
11 changes: 8 additions & 3 deletions src/bases/test/mocks/SemaphoreAuthMock.sol
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,17 @@ contract SemaphoreAuthMock is FirmBase, SemaphoreAuth {
}

// Make ISemaphore internal functions public for testing

function semaphoreCheckCall(address target, uint256 value, bytes memory data, bool isDelegateCall) public view {
_semaphoreCheckCall(target, value, data, isDelegateCall);
}

function semaphoreCheckCalls(address[] memory targets, uint256[] memory values, bytes[] memory datas, bool isDelegateCall) public view {
function semaphoreCheckCalls(
address[] memory targets,
uint256[] memory values,
bytes[] memory datas,
bool isDelegateCall
) public view {
_semaphoreCheckCalls(targets, values, datas, isDelegateCall);
}

Expand All @@ -31,4 +36,4 @@ contract SemaphoreAuthMock is FirmBase, SemaphoreAuth {
) public pure returns (address[] memory, uint256[] memory, bytes[] memory) {
return _filterCallsToTarget(filteredTarget, targets, values, calldatas);
}
}
}
14 changes: 10 additions & 4 deletions src/bases/test/mocks/SemaphoreStub.sol
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ pragma solidity 0.8.17;
import {ISemaphore} from "src/semaphore/interfaces/ISemaphore.sol";

contract SemaphoreStub is ISemaphore {
mapping (address => bool) public disallowedTargets;
mapping (address => bool) public disallowedCallers;
mapping(address => bool) public disallowedTargets;
mapping(address => bool) public disallowedCallers;

function setDisallowed(address addr, bool targetOrCaller) external {
if (targetOrCaller) {
Expand All @@ -25,12 +25,18 @@ contract SemaphoreStub is ISemaphore {
return true;
}

function canPerformMany(address caller, address[] calldata targets, uint256[] calldata values, bytes[] calldata calldatas, bool isDelegateCall) external view returns (bool) {
function canPerformMany(
address caller,
address[] calldata targets,
uint256[] calldata values,
bytes[] calldata calldatas,
bool isDelegateCall
) external view returns (bool) {
for (uint256 i = 0; i < targets.length; i++) {
if (!canPerform(caller, targets[i], values[i], calldatas[i], isDelegateCall)) {
return false;
}
}
return true;
}
}
}
2 changes: 1 addition & 1 deletion src/captable/Captable.sol
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,7 @@ contract Captable is FirmBase, BouncerChecker, ICaptableVotes {
if (classBalance == 0) {
revert AccountIsNonHolder(account, classId);
}

_setController(account, classId, classBalance, controller, controllerParams);
}

Expand Down
8 changes: 5 additions & 3 deletions src/captable/controllers/test/AccountControllerTest.sol
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,11 @@ abstract contract AccountControllerTest is FirmTest {

function setUp() public virtual {
safe = new SafeStub();
captable = Captable(createProxy(new Captable(), abi.encodeCall(Captable.initialize, ("TestCo", safe, address(0)))));
captable =
Captable(createProxy(new Captable(), abi.encodeCall(Captable.initialize, ("TestCo", safe, address(0)))));
vm.prank(address(safe));
(classId, token) = captable.createClass("Common", "TST.A", authorizedAmount, NO_CONVERSION_FLAG, 1, ALLOW_ALL_BOUNCER);
(classId, token) =
captable.createClass("Common", "TST.A", authorizedAmount, NO_CONVERSION_FLAG, 1, ALLOW_ALL_BOUNCER);
}

function controller() internal view virtual returns (AccountController);
Expand All @@ -33,4 +35,4 @@ abstract contract AccountControllerTest is FirmTest {
assertEq(address(controller().safe()), address(safe));
assertUnsStrg(address(controller()), "firm.safeaware.safe", address(safe));
}
}
}
56 changes: 37 additions & 19 deletions src/captable/controllers/test/VestingController.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,11 @@ contract VestingControllerTest is AccountControllerTest {
vm.warp(0);

roles = new RolesStub();
vesting = VestingController(createProxy(new VestingController(), abi.encodeCall(VestingController.initialize, (captable, roles, address(0)))));
vesting = VestingController(
createProxy(
new VestingController(), abi.encodeCall(VestingController.initialize, (captable, roles, address(0)))
)
);

roles.setRole(REVOKER, REVOKER_ROLE, true);
}
Expand Down Expand Up @@ -65,22 +69,36 @@ contract VestingControllerTest is AccountControllerTest {

function testCannotAddAccountWithInvalidParameters() public {
vm.startPrank(address(captable));

vm.expectRevert(abi.encodeWithSelector(VestingController.InvalidVestingParameters.selector));
vesting.addAccount(HOLDER, classId, authorizedAmount - 1, abi.encode(VestingController.VestingParams({
startDate: 21,
cliffDate: 20,
endDate: 30,
revoker: roleFlag(REVOKER_ROLE)
})));
vesting.addAccount(
HOLDER,
classId,
authorizedAmount - 1,
abi.encode(
VestingController.VestingParams({
startDate: 21,
cliffDate: 20,
endDate: 30,
revoker: roleFlag(REVOKER_ROLE)
})
)
);

vm.expectRevert(abi.encodeWithSelector(VestingController.InvalidVestingParameters.selector));
vesting.addAccount(HOLDER, classId, authorizedAmount - 1, abi.encode(VestingController.VestingParams({
startDate: 10,
cliffDate: 31,
endDate: 30,
revoker: roleFlag(REVOKER_ROLE)
})));
vesting.addAccount(
HOLDER,
classId,
authorizedAmount - 1,
abi.encode(
VestingController.VestingParams({
startDate: 10,
cliffDate: 31,
endDate: 30,
revoker: roleFlag(REVOKER_ROLE)
})
)
);

vm.stopPrank();
}
Expand All @@ -106,11 +124,11 @@ contract VestingControllerTest is AccountControllerTest {
}

function testRevokerCanRevokeAtThisTime() public {
(,uint40 cliffDate,) = testCaptableAddsAccount();
(, uint40 cliffDate,) = testCaptableAddsAccount();
vm.warp(cliffDate);
vm.prank(REVOKER);
vesting.revokeVesting(HOLDER, classId);

assertEq(captable.balanceOf(HOLDER, classId), issuedAmount / 2);

assertAccountWasCleanedUp();
Expand All @@ -121,14 +139,14 @@ contract VestingControllerTest is AccountControllerTest {
vm.warp(startDate);
vm.prank(REVOKER);
vesting.revokeVesting(HOLDER, classId, cliffDate);

assertEq(captable.balanceOf(HOLDER, classId), issuedAmount / 2);

assertAccountWasCleanedUp();
}

function testRevokerCantRevokeInThePast() public {
(,uint40 cliffDate,) = testCaptableAddsAccount();
(, uint40 cliffDate,) = testCaptableAddsAccount();
vm.warp(cliffDate);
vm.prank(REVOKER);
vm.expectRevert(abi.encodeWithSelector(VestingController.EffectiveDateInThePast.selector));
Expand Down Expand Up @@ -183,4 +201,4 @@ contract VestingControllerTest is AccountControllerTest {
vm.expectRevert(abi.encodeWithSelector(AccountController.AccountDoesntExist.selector));
vesting.revokeVesting(HOLDER, classId, 1);
}
}
}
5 changes: 3 additions & 2 deletions src/captable/test/Captable.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -145,11 +145,12 @@ contract CaptableInitTest is BaseCaptableTest {

for (uint256 i = 0; i < CLASSES_LIMIT; i++) {
vm.prank(address(safe));
(uint256 classId, EquityToken token) = captable.createClass("", "", maxAuthorizable, NO_CONVERSION_FLAG, maxVotingWeight, ALLOW_ALL_BOUNCER);
(uint256 classId, EquityToken token) =
captable.createClass("", "", maxAuthorizable, NO_CONVERSION_FLAG, maxVotingWeight, ALLOW_ALL_BOUNCER);

vm.prank(address(safe));
captable.issue(HOLDER1, classId, maxAuthorizable);

vm.prank(address(HOLDER1));
token.delegate(HOLDER1);
}
Expand Down
4 changes: 2 additions & 2 deletions src/captable/test/EquityToken.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,13 @@ contract EquityTokenTest is BaseCaptableTest {
token.forcedTransfer(HOLDER2, HOLDER1, 1);
assertEq(token.balanceOf(HOLDER1), HOLDER_BALANCE + 1);
assertEq(token.balanceOf(HOLDER2), 0);

vm.stopPrank();
}

function testNonCaptableCannotPerformAdminActions() public {
bytes memory onlyCaptableError = abi.encodeWithSelector(EquityToken.UnauthorizedNotCaptable.selector);

vm.startPrank(address(safe));

vm.expectRevert(onlyCaptableError);
Expand Down
8 changes: 5 additions & 3 deletions src/captable/test/lib/BouncerFlags.sol
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
// SPDX-License-Identifier: GPL-3.0-or-later
pragma solidity 0.8.17;

import {IBouncer, EmbeddedBouncerType, EMBEDDED_BOUNCER_FLAG_TYPE, AddressUint8FlagsLib} from "../../BouncerChecker.sol";
import {
IBouncer, EmbeddedBouncerType, EMBEDDED_BOUNCER_FLAG_TYPE, AddressUint8FlagsLib
} from "../../BouncerChecker.sol";

function bouncerFlag(EmbeddedBouncerType bouncer) pure returns (IBouncer) {
return IBouncer(AddressUint8FlagsLib.toFlag(uint8(bouncer), EMBEDDED_BOUNCER_FLAG_TYPE));
}
return IBouncer(AddressUint8FlagsLib.toFlag(uint8(bouncer), EMBEDDED_BOUNCER_FLAG_TYPE));
}
Loading