Closed horsefacts closed 1 year ago
Coverage after merging horsefacts/ownership-transfer-cleanup into main will be
98.13% |
---|
File | Stmts | Branches | Funcs | Lines | Uncovered Lines |
---|---|---|---|---|---|
src | |||||
Bundler.sol | 100% | 100% | 100% | 100% | |
FnameResolver.sol | 96.30% | 75% | 100% | 100% | 122 |
IdRegistry.sol | 100% | 100% | 100% | 100% | |
KeyRegistry.sol | 100% | 100% | 100% | 100% | |
StorageRent.sol | 98.90% | 98.48% | 100% | 98.96% | 532, 534 |
src/lib | |||||
TransferHelper.sol | 0% | 0% | 0% | 0% | 15, 15, 15, 25–26, 26, 26 |
Motivation
In the current
TestSuiteSetup
contract, we assignaddress(this)
(i.e. the test contract address) asowner
and use this value throughout the tests. Since the test contract is the owner, we don't need to usevm.prank
to make authenticated calls.This is convenient, but it makes it harder to test that ownership is configured correctly. In practice the owner will not be the deployer: since we'll be deploying contracts using a CREATE2 factory, we want to transfer ownership to an explicit address at construction time. If the test contract itself is set as the
owner
, it's difficult to tell whether we're relying on the default behavior ofOwnable
or transferring ownership correctly. (And in fact, inIdRegistry
, we weren't transferring ownership to the_owner
arg in the constructor, see #280 ).Change Summary
Generate an explicit address for
owner
usingmakeAddr
and use it throughout the tests. Replaceadmin
withowner
in key registry and resolver tests.Merge Checklist
Choose all relevant options below by adding an
x
now or at any time before submitting for reviewAdditional Context
Fixes unused
_owner
argument inIdRegistry
constructor.Close #280
PR-Codex overview
Focus of this PR:
This PR focuses on making changes to several contracts and tests related to the IdRegistry, FnameResolver, and Bundler functionality.
Detailed summary:
TestSuiteSetup.sol
, theowner
variable is changed tomakeAddr("owner")
.IdRegistry.sol
, the constructor is modified to include a call to_transferOwnership(_owner)
.KeyRegistryTestSuite.sol
, theadmin
variable is changed tomakeAddr("admin")
and thesetUp
function is modified to include a call tovm.prank(owner)
.IdRegistryTestSuite.sol
, the_registerWithRecovery
function is modified to include a call tovm.prank(owner)
.IdRegistry.gas.t.sol
, thetestGasRegister
andtestGasRegisterForAndRecover
functions are modified to include a call tovm.prank(owner)
.Bundler.gas.t.sol
, thetestGasRegisterWithSig
,testGasTrustedRegister
, andtestGasTrustedBatchRegister
functions are modified to include a call tovm.prank(owner)
.FnameResolverTestSuite.sol
, the import statements forforge-std/Test.sol
andopenzeppelin/contracts/utils/cryptography/EIP712.sol
are removed and replaced with import statements forTestSuiteSetup.sol
andFnameResolverHarness
.IdRegistry.owner.t.sol
, several functions are modified to include a call tovm.prank(owner)
.FnameResolver.t.sol
, thetestInitialOwner
function is modified to check forowner
instead ofadmin
.IdRegistry.t.sol
, several functions are modified to include a call tovm.prank(owner)
.Bundler.t.sol
, thetestFuzzTrustedRegister
andtestFuzzCannotTrustedRegisterWhenPaused
functions are modified to include a call tovm.prank(owner)
.