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.

Sign up for GitHub

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

add ownerCount method #60

Merged
merged 2 commits into from
Apr 19, 2024
Merged

add ownerCount method #60

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8d25bc4
add ownerCount method
wilsoncusack Apr 19, 2024
5e90754
do not call twice
wilsoncusack Apr 19, 2024
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
116 changes: 58 additions & 58 deletions .gas-snapshot
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,72 +1,72 @@
AddOwnerAddressTest:testEmitsAddOwner() (gas: 92402)
AddOwnerAddressTest:testIncreasesOwnerIndex() (gas: 90676)
AddOwnerAddressTest:testRevertsIfAlreadyOwner() (gas: 93191)
AddOwnerAddressTest:testRevertsIfCalledByNonOwner() (gas: 11764)
AddOwnerAddressTest:testSetsIsOwner() (gas: 90470)
AddOwnerAddressTest:testSetsOwnerAtIndex() (gas: 98970)
AddOwnerPublicKeyTest:testEmitsAddOwner() (gas: 115537)
AddOwnerPublicKeyTest:testFuzzIsOwnerPublicKey(bytes32,bytes32) (runs: 256, μ: 115328, ~: 115328)
AddOwnerPublicKeyTest:testRevertsIfAlreadyOwner() (gas: 116355)
AddOwnerPublicKeyTest:testRevertsIfCalledByNonOwner() (gas: 11754)
AddOwnerPublicKeyTest:testSetsIsOwner() (gas: 113769)
AddOwnerPublicKeyTest:testSetsOwnerAtIndex() (gas: 128616)
CoinbaseSmartWallet1271InputGeneratorTest:testGetReplaySafeHashForDeployedAccount() (gas: 308446)
CoinbaseSmartWallet1271InputGeneratorTest:testGetReplaySafeHashForUndeployedAccount() (gas: 291475)
CoinbaseSmartWalletFactoryTest:testDeployDeterministicPassValues() (gas: 267250)
CoinbaseSmartWalletFactoryTest:test_CreateAccount_ReturnsPredeterminedAddress_WhenAccountAlreadyExists() (gas: 286914)
CoinbaseSmartWalletFactoryTest:test_RevertsIfLength32ButLargerThanAddress() (gas: 298636)
CoinbaseSmartWalletFactoryTest:test_constructor_setsImplementation(address) (runs: 256, μ: 314355, ~: 314355)
CoinbaseSmartWalletFactoryTest:test_createAccountDeploysToPredeterminedAddress() (gas: 268601)
CoinbaseSmartWalletFactoryTest:test_createAccountSetsOwnersCorrectly() (gas: 277291)
CoinbaseSmartWalletFactoryTest:test_exitIfAccountIsAlreadyInitialized() (gas: 267718)
AddOwnerAddressTest:testEmitsAddOwner() (gas: 92424)
AddOwnerAddressTest:testIncreasesOwnerIndex() (gas: 90698)
AddOwnerAddressTest:testRevertsIfAlreadyOwner() (gas: 93235)
AddOwnerAddressTest:testRevertsIfCalledByNonOwner() (gas: 11786)
AddOwnerAddressTest:testSetsIsOwner() (gas: 90492)
AddOwnerAddressTest:testSetsOwnerAtIndex() (gas: 98992)
AddOwnerPublicKeyTest:testEmitsAddOwner() (gas: 115559)
AddOwnerPublicKeyTest:testFuzzIsOwnerPublicKey(bytes32,bytes32) (runs: 256, μ: 115372, ~: 115372)
AddOwnerPublicKeyTest:testRevertsIfAlreadyOwner() (gas: 116399)
AddOwnerPublicKeyTest:testRevertsIfCalledByNonOwner() (gas: 11776)
AddOwnerPublicKeyTest:testSetsIsOwner() (gas: 113813)
AddOwnerPublicKeyTest:testSetsOwnerAtIndex() (gas: 128638)
CoinbaseSmartWallet1271InputGeneratorTest:testGetReplaySafeHashForDeployedAccount() (gas: 308479)
CoinbaseSmartWallet1271InputGeneratorTest:testGetReplaySafeHashForUndeployedAccount() (gas: 291508)
CoinbaseSmartWalletFactoryTest:testDeployDeterministicPassValues() (gas: 267283)
CoinbaseSmartWalletFactoryTest:test_CreateAccount_ReturnsPredeterminedAddress_WhenAccountAlreadyExists() (gas: 286935)
CoinbaseSmartWalletFactoryTest:test_RevertsIfLength32ButLargerThanAddress() (gas: 298669)
CoinbaseSmartWalletFactoryTest:test_constructor_setsImplementation(address) (runs: 256, μ: 313355, ~: 313355)
CoinbaseSmartWalletFactoryTest:test_createAccountDeploysToPredeterminedAddress() (gas: 268634)
CoinbaseSmartWalletFactoryTest:test_createAccountSetsOwnersCorrectly() (gas: 277324)
CoinbaseSmartWalletFactoryTest:test_exitIfAccountIsAlreadyInitialized() (gas: 267739)
CoinbaseSmartWalletFactoryTest:test_implementation_returnsExpectedAddress() (gas: 7676)
CoinbaseSmartWalletFactoryTest:test_initCodeHash() (gas: 7912)
CoinbaseSmartWalletFactoryTest:test_revertsIfNoOwners() (gas: 29232)
ERC1271Test:test_returnsExpectedDomainHashWhenProxy() (gas: 29198)
ERC1271Test:test_static() (gas: 3235744)
ERC1271Test:test_returnsExpectedDomainHashWhenProxy() (gas: 29243)
ERC1271Test:test_static() (gas: 3238027)
MultiOwnableInitializeTest:testRevertsIfLength32ButLargerThanAddress() (gas: 81111)
MultiOwnableInitializeTest:testRevertsIfLength32NotAddress() (gas: 81092)
MultiOwnableInitializeTest:testRevertsIfLengthNot32Or64() (gas: 103395)
RemoveLastOwnerTest:test_emitsRemoveOwner() (gas: 50337)
RemoveLastOwnerTest:test_removesOwner() (gas: 49609)
RemoveLastOwnerTest:test_removesOwnerAtIndex() (gas: 49562)
RemoveLastOwnerTest:test_emitsRemoveOwner() (gas: 50363)
RemoveLastOwnerTest:test_removesOwner() (gas: 49657)
RemoveLastOwnerTest:test_removesOwnerAtIndex() (gas: 49588)
RemoveLastOwnerTest:test_revert_whenCalledByNonOwner(address) (runs: 256, μ: 19094, ~: 19094)
RemoveLastOwnerTest:test_revert_whenNoOwnerAtIndex() (gas: 48379)
RemoveLastOwnerTest:test_revert_whenWrongOwnerAtIndex() (gas: 34153)
RemoveLastOwnerTest:test_reverts_whenNotLastOwner() (gas: 123412)
RemoveOwnerAtIndexTest:test_emitsRemoveOwner() (gas: 55524)
RemoveOwnerAtIndexTest:test_removesOwner() (gas: 54784)
RemoveOwnerAtIndexTest:test_removesOwnerAtIndex() (gas: 54540)
RemoveLastOwnerTest:test_revert_whenNoOwnerAtIndex() (gas: 48407)
RemoveLastOwnerTest:test_revert_whenWrongOwnerAtIndex() (gas: 34181)
RemoveLastOwnerTest:test_reverts_whenNotLastOwner() (gas: 123462)
RemoveOwnerAtIndexTest:test_emitsRemoveOwner() (gas: 55549)
RemoveOwnerAtIndexTest:test_removesOwner() (gas: 54828)
RemoveOwnerAtIndexTest:test_removesOwnerAtIndex() (gas: 54566)
RemoveOwnerAtIndexTest:test_revert_whenCalledByNonOwner(address) (runs: 256, μ: 19145, ~: 19145)
RemoveOwnerAtIndexTest:test_revert_whenNoOwnerAtIndex() (gas: 33372)
RemoveOwnerAtIndexTest:test_revert_whenWrongOwnerAtIndex() (gas: 36694)
RemoveOwnerAtIndexTest:test_reverts_ifIsLastOwner() (gas: 7471805)
RemoveOwnerAtIndexTest:test_revert_whenNoOwnerAtIndex() (gas: 33406)
RemoveOwnerAtIndexTest:test_revert_whenWrongOwnerAtIndex() (gas: 36728)
RemoveOwnerAtIndexTest:test_reverts_ifIsLastOwner() (gas: 7647492)
TestCanSkipChainIdValidation:test_approvedSelectorsReturnTrue() (gas: 17781)
TestCanSkipChainIdValidation:test_otherSelectorsReturnFalse() (gas: 12579)
TestExecuteWithoutChainIdValidation:testExecute() (gas: 424837)
TestExecuteWithoutChainIdValidation:testExecuteBatch() (gas: 728897)
TestExecuteWithoutChainIdValidation:testExecuteBatch(uint256) (runs: 256, μ: 3592913, ~: 3463925)
TestExecuteWithoutChainIdValidation:test__codesize() (gas: 49920)
TestExecuteWithoutChainIdValidation:test__codesize() (gas: 50155)
TestExecuteWithoutChainIdValidation:testExecute() (gas: 424882)
TestExecuteWithoutChainIdValidation:testExecuteBatch() (gas: 728942)
TestExecuteWithoutChainIdValidation:testExecuteBatch(uint256) (runs: 256, μ: 3590200, ~: 3466169)
TestExecuteWithoutChainIdValidation:test__codesize() (gas: 49931)
TestExecuteWithoutChainIdValidation:test__codesize() (gas: 50166)
TestExecuteWithoutChainIdValidation:test_revertsWithReservedNonce() (gas: 81983)
TestExecuteWithoutChainIdValidation:test_reverts_whenCallerNotEntryPoint() (gas: 11053)
TestExecuteWithoutChainIdValidation:test_reverts_whenOneCallReverts() (gas: 467715)
TestExecuteWithoutChainIdValidation:test_reverts_whenOneSelectorNotApproved() (gas: 179684)
TestExecuteWithoutChainIdValidation:test_reverts_whenSelectorNotApproved() (gas: 106758)
TestExecuteWithoutChainIdValidation:test_succeeds_whenSelectorAllowed() (gas: 423916)
TestImplementation:testImplementation() (gas: 12623)
TestInitialize:testInitialize() (gas: 21034)
TestInitialize:test_cannotInitImplementation() (gas: 2885066)
TestIsValidSignature:testReturnsInvalidIfPasskeySigButWrongOwnerLength() (gas: 40187)
TestIsValidSignature:testRevertsIfEthereumSignatureButWrongOwnerLength() (gas: 24040)
TestIsValidSignature:testRevertsIfOwnerIsInvalidEthereumAddress() (gas: 22021)
TestIsValidSignature:testSmartWalletSigner() (gas: 3166717)
TestIsValidSignature:testValidateSignatureWithEOASigner() (gas: 24922)
TestIsValidSignature:testValidateSignatureWithEOASignerFailsWithWrongSigner() (gas: 23877)
TestIsValidSignature:testValidateSignatureWithPasskeySigner() (gas: 423238)
TestIsValidSignature:testValidateSignatureWithPasskeySignerFailsBadOwnerIndex() (gas: 35664)
TestIsValidSignature:testValidateSignatureWithPasskeySignerFailsWithWrongBadSignature() (gas: 430704)
TestUpgradeToAndCall:testUpgradeToAndCall() (gas: 25477)
TestExecuteWithoutChainIdValidation:test_reverts_whenCallerNotEntryPoint() (gas: 11031)
TestExecuteWithoutChainIdValidation:test_reverts_whenOneCallReverts() (gas: 467783)
TestExecuteWithoutChainIdValidation:test_reverts_whenOneSelectorNotApproved() (gas: 179662)
TestExecuteWithoutChainIdValidation:test_reverts_whenSelectorNotApproved() (gas: 106736)
TestExecuteWithoutChainIdValidation:test_succeeds_whenSelectorAllowed() (gas: 423939)
TestImplementation:testImplementation() (gas: 12558)
TestInitialize:testInitialize() (gas: 21012)
TestInitialize:test_cannotInitImplementation() (gas: 2887326)
TestIsValidSignature:testReturnsInvalidIfPasskeySigButWrongOwnerLength() (gas: 40165)
TestIsValidSignature:testRevertsIfEthereumSignatureButWrongOwnerLength() (gas: 24018)
TestIsValidSignature:testRevertsIfOwnerIsInvalidEthereumAddress() (gas: 21999)
TestIsValidSignature:testSmartWalletSigner() (gas: 3168925)
TestIsValidSignature:testValidateSignatureWithEOASigner() (gas: 24900)
TestIsValidSignature:testValidateSignatureWithEOASignerFailsWithWrongSigner() (gas: 23855)
TestIsValidSignature:testValidateSignatureWithPasskeySigner() (gas: 423216)
TestIsValidSignature:testValidateSignatureWithPasskeySignerFailsBadOwnerIndex() (gas: 35642)
TestIsValidSignature:testValidateSignatureWithPasskeySignerFailsWithWrongBadSignature() (gas: 430682)
TestUpgradeToAndCall:testUpgradeToAndCall() (gas: 25499)
TestValidateUserOp:test_reverts_whenReplayableNonceKeyInvalidForSelector() (gas: 14187)
TestValidateUserOp:test_reverts_whenSelectorInvalidForReplayableNonceKey() (gas: 14452)
TestValidateUserOp:test_succeedsWithEOASigner() (gas: 447938)
Expand Down
14 changes: 10 additions & 4 deletions src/MultiOwnable.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,8 +120,7 @@ contract MultiOwnable {
/// @param index The index of the owner to be removed.
/// @param owner The ABI encoded bytes of the owner to be removed.
function removeOwnerAtIndex(uint256 index, bytes calldata owner) external virtual onlyOwner {
MultiOwnableStorage storage $ = _getMultiOwnableStorage();
if ($.nextOwnerIndex - $.removedOwnersCount == 1) {
if (ownerCount() == 1) {
revert LastOwner();
}

Expand All @@ -137,8 +136,7 @@ contract MultiOwnable {
/// @param index The index of the owner to be removed.
/// @param owner The ABI encoded bytes of the owner to be removed.
function removeLastOwner(uint256 index, bytes calldata owner) external virtual onlyOwner {
MultiOwnableStorage storage $ = _getMultiOwnableStorage();
uint256 ownersRemaining = $.nextOwnerIndex - $.removedOwnersCount;
uint256 ownersRemaining = ownerCount();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can get rid of this stack variable and do the same thing as in `removeOwnerAtIndex():

if (ownerCount() > 1) {
    revert NotLastOwner(ownersRemaining);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but then I would need to call ownerCount again here revert NotLastOwner(ownersRemaining);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be revert NotLastOwner(ownerCount());

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes right, I missed that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But yeah could debate: do we penalize gas on the non revert path in order to save on revert?

if (ownersRemaining > 1) {
revert NotLastOwner(ownersRemaining);
}
Expand Down Expand Up @@ -190,6 +188,14 @@ contract MultiOwnable {
return _getMultiOwnableStorage().nextOwnerIndex;
}

/// @notice Returns the current number of owners
///
/// @return The current owner count
function ownerCount() public view virtual returns (uint256) {
MultiOwnableStorage storage $ = _getMultiOwnableStorage();
return $.nextOwnerIndex - $.removedOwnersCount;
}

/// @notice Tracks the number of owners removed, used with nextOwnerIndex to avoid removing all owners
///
/// @return The number of owners that have been removed.
Expand Down
3 changes: 3 additions & 0 deletions test/MultiOwnable/RemoveOwnerAtIndex.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,17 @@ contract RemoveOwnerAtIndexTest is RemoveOwnerBaseTest {
mock.init(initialOwners);
assertEq(mock.nextOwnerIndex(), 1);
assertEq(mock.removedOwnersCount(), 0);
assertEq(mock.ownerCount(), 1);
vm.startPrank(firstOnwer);
for (uint256 i; i < owners; i++) {
mock.addOwnerAddress(makeAddr(string(abi.encodePacked(i))));
assertEq(mock.nextOwnerIndex(), i + 2);
assertEq(mock.ownerCount(), i + 2);
}
for (uint256 i = 1; i < owners + 1; i++) {
mock.removeOwnerAtIndex(i, abi.encode(makeAddr(string(abi.encodePacked(i - 1)))));
assertEq(mock.removedOwnersCount(), i);
assertEq(mock.ownerCount(), owners - i + 1);
}
vm.expectRevert(MultiOwnable.LastOwner.selector);
mock.removeOwnerAtIndex(0, abi.encode(firstOnwer));
Expand Down
Loading