Skip to content

Commit

Permalink
feat: minor refactor
Browse files Browse the repository at this point in the history
  • Loading branch information
@iavl
iavl committed Mar 27, 2024
1 parent ae9ecee commit ce64166
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 84 deletions.
97 changes: 25 additions & 72 deletions src/ProxyAdminMultiSig.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,13 @@ import {IErrors} from "./interfaces/IErrors.sol";
import {Const} from "./libraries/Const.sol";
import {Events} from "./libraries/Events.sol";
import {IProxy} from "./interfaces/IProxy.sol";
import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";
import {Math} from "@openzeppelin/contracts/utils/math/Math.sol";

contract ProxyAdminMultiSig is IErrors {
using EnumerableSet for EnumerableSet.AddressSet;
using EnumerableSet for EnumerableSet.UintSet;

struct Proposal {
uint256 proposalId;
string proposalType; // "ChangeAdmin" or "Upgrade" or "UpgradeToAndCall"
Expand All @@ -22,17 +27,17 @@ contract ProxyAdminMultiSig is IErrors {
uint256 public constant MAX_UINT256 = type(uint256).max;

// multi-sig wallet
mapping(address => address) internal _owners;
EnumerableSet.AddressSet internal _owners;
uint256 internal _ownersCount;
uint256 internal _threshold;

// proposals
uint256 internal _proposalCount;
mapping(uint256 => Proposal) internal _proposals;
uint256[] internal _pendingProposalIds;
EnumerableSet.UintSet internal _pendingProposalIds;

modifier onlyOwner() {
if (_owners[msg.sender] == address(0)) {
if (!_owners.contains(msg.sender)) {
revert NotOwner();
}
_;
Expand All @@ -43,26 +48,22 @@ contract ProxyAdminMultiSig is IErrors {
if (threshold == 0) {
revert ThresholdIsZero();
}
if (threshold > owners.length) {
revert ThresholdExceedsOwnersCount(threshold, owners.length);
}

// initialize owners
address currentOwner = Const.SENTINEL_OWNER;
for (uint256 i = 0; i < owners.length; i++) {
address owner = owners[i];
if (owner == address(0) || owner == Const.SENTINEL_OWNER || currentOwner == owner) {
revert InvalidOwner();
if (owner == address(0)) {
revert ZeroAddress();
}
if (_owners[owner] != address(0)) {
revert OwnerExists();
if (!_owners.add(owner)) {
revert DuplicatedOwner(owner);
}
_owners[currentOwner] = owner;
currentOwner = owner;
}
_owners[currentOwner] = Const.SENTINEL_OWNER;
_ownersCount = owners.length;
_ownersCount = _owners.length();
_threshold = threshold;
if (threshold > _ownersCount) {
revert ThresholdExceedsOwnersCount(threshold, _ownersCount);
}

emit Events.Setup(msg.sender, owners, _ownersCount, threshold);
}
Expand All @@ -89,7 +90,7 @@ contract ProxyAdminMultiSig is IErrors {
_proposals[proposalId].data = data;
_proposals[proposalId].approvalCount = 0;
_proposals[proposalId].status = Const.STATUS_PENDING;
_pendingProposalIds.push(proposalId);
_pendingProposalIds.add(proposalId);

emit Events.Proposed(proposalId, proxy, proposalType, newAdminOrImplementation, data);
}
Expand Down Expand Up @@ -135,7 +136,7 @@ contract ProxyAdminMultiSig is IErrors {
}

// update proposal
_deletePendingProposalId(proposalId);
_pendingProposalIds.remove(proposalId);
_proposals[proposalId].status = Const.STATUS_EXECUTED;
}

Expand All @@ -145,19 +146,19 @@ contract ProxyAdminMultiSig is IErrors {
revert NotPendingProposal();
}

_deletePendingProposalId(proposalId);
_pendingProposalIds.remove(proposalId);
_proposals[proposalId].status = Const.STATUS_DELETED;

emit Events.Deleted(msg.sender, proposalId);
}

/// @dev returns pending proposals
function getPendingProposals() external view returns (Proposal[] memory results) {
uint256 len = _pendingProposalIds.length;
uint256 len = _pendingProposalIds.length();

results = new Proposal[](len);
for (uint256 i = 0; i < len; i++) {
uint256 pid = _pendingProposalIds[i];
uint256 pid = _pendingProposalIds.at(i);
results[i] = _proposals[pid];
}
}
Expand All @@ -166,7 +167,7 @@ contract ProxyAdminMultiSig is IErrors {
function getAllProposals(uint256 offset, uint256 limit) external view returns (Proposal[] memory results) {
if (offset >= _proposalCount) return results;

uint256 len = _min(limit, _proposalCount - offset);
uint256 len = Math.min(limit, _proposalCount - offset);

results = new Proposal[](len);
for (uint256 i = offset; i < offset + len; i++) {
Expand All @@ -179,7 +180,7 @@ contract ProxyAdminMultiSig is IErrors {
function getWalletDetail() external view returns (uint256 threshold, uint256 ownersCount, address[] memory owners) {
threshold = _threshold;
ownersCount = _ownersCount;
owners = _getOwners();
owners = _owners.values();
}

/// @dev get proposal count
Expand All @@ -189,38 +190,7 @@ contract ProxyAdminMultiSig is IErrors {

/// @dev checks if an address is an owner
function isOwner(address owner) external view returns (bool) {
return owner != Const.SENTINEL_OWNER && _owners[owner] != address(0);
}

/// @dev deletes a pending proposal by proposalId
function _deletePendingProposalId(uint256 proposalId) internal {
// find index to be deleted
uint256 index = _getPendingProposalIndex(proposalId);

// if index not equal to MAX_UINT256, it means the proposalId is found
if (index != MAX_UINT256) {
uint256 lastIndex = _pendingProposalIds.length - 1;
if (lastIndex != index) {
_pendingProposalIds[index] = _pendingProposalIds[lastIndex];
}

// delete the last slot
_pendingProposalIds.pop();
}
}

/// @dev returns all owners
function _getOwners() internal view returns (address[] memory) {
address[] memory array = new address[](_ownersCount);

uint256 index = 0;
address currentOwner = _owners[Const.SENTINEL_OWNER];
while (currentOwner != Const.SENTINEL_OWNER) {
array[index] = currentOwner;
currentOwner = _owners[currentOwner];
index++;
}
return array;
return _owners.contains(owner);
}

/// @dev checks if an owner has approved a proposal
Expand All @@ -239,24 +209,7 @@ contract ProxyAdminMultiSig is IErrors {

/// @dev checks if a proposal is pending
function _isPendingProposal(uint256 proposalId) internal view returns (bool) {
uint256 index = _getPendingProposalIndex(proposalId);
return index != MAX_UINT256;
}

/// @dev gets the index of a pending proposal, return MAX_UINT256 if not found,
function _getPendingProposalIndex(uint256 proposalId) internal view returns (uint256 index) {
index = MAX_UINT256;
for (uint256 i = 0; i < _pendingProposalIds.length; i++) {
if (proposalId == _pendingProposalIds[i]) {
index = i;
break;
}
}
}

/// @dev returns the smallest of two numbers.
function _min(uint256 a, uint256 b) internal pure returns (uint256) {
return a < b ? a : b;
return _pendingProposalIds.contains(proposalId);
}

/// @dev returns true if the two strings are equal.
Expand Down
3 changes: 2 additions & 1 deletion src/interfaces/IErrors.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@ interface IErrors {
error NotOwner();
error ThresholdIsZero();
error ThresholdExceedsOwnersCount(uint256 threshold, uint256 ownersCount);
error InvalidOwner();
error DuplicatedOwner(address owner);
error ZeroAddress();
error OwnerExists();
error UnexpectedProposalType();
error NotPendingProposal();
Expand Down
15 changes: 4 additions & 11 deletions test/ProxyAdminMultiSig.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,19 +81,12 @@ contract MultiSigTest is IErrors, Test, Utils {
vm.expectRevert(abi.encodeWithSelector(ThresholdExceedsOwnersCount.selector, 4, 3));
multiSig = new ProxyAdminMultiSig(ownersArr3, 4);

// [alice, bob, alice] and [alice, alice, bob]
// replicated owners
vm.expectRevert(InvalidOwner.selector);
multiSig = new ProxyAdminMultiSig(replicatedOwners, 1);

// owner can't be 0x0 or 0x1
vm.expectRevert(InvalidOwner.selector);
// owner can't be 0x0
vm.expectRevert(ZeroAddress.selector);
multiSig = new ProxyAdminMultiSig(zeroOwners, 1);

vm.expectRevert(InvalidOwner.selector);
multiSig = new ProxyAdminMultiSig(sentinelOwners, 1);

vm.expectRevert(OwnerExists.selector);
// duplicate owner
vm.expectRevert(abi.encodeWithSelector(DuplicatedOwner.selector, alice));
multiSig = new ProxyAdminMultiSig(existsOwners, 1);
}

Expand Down

0 comments on commit ce64166

Please sign in to comment.