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

[Upgrade] Add owner getter and setter to Asset and Catalyst #1345

Draft
wants to merge 8 commits into
base: feat/hardhat-updates-verification
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 4 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
18 changes: 17 additions & 1 deletion packages/asset/contracts/Asset.sol
Original file line number Diff line number Diff line change
Expand Up @@ -384,5 +384,21 @@ contract Asset is
return "ASSET";
}

uint256[49] private __gap;
address private _owner;
Copy link
Contributor

Choose a reason for hiding this comment

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

Move to line 59?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I have declared it there so its all close together, but yeah will move it up


/// @notice Returns the owner of the contract
/// @return address of the owner
function owner() external view returns (address) {
return _owner;
}

/// @notice Sets the owner of the contract
/// @param newOwner address of the new owner
function transferOwnership(address newOwner) external {
Copy link
Contributor

Choose a reason for hiding this comment

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

why not onlyRole(DEFAULT_ADMIN_ROLE) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the main reason is to be able to provide custom revert message.
Otherwise we could switch to using the modifier.

require(hasRole(DEFAULT_ADMIN_ROLE, _msgSender()), "Asset: Unauthorized");
_owner = newOwner;
emit OwnershipTransferred(_owner, newOwner);
}

uint256[48] private __gap;
}
18 changes: 17 additions & 1 deletion packages/asset/contracts/Catalyst.sol
Original file line number Diff line number Diff line change
Expand Up @@ -324,5 +324,21 @@ contract Catalyst is
return "CATALYST";
}

uint256[49] private __gap;
address private _owner;
Copy link
Contributor

Choose a reason for hiding this comment

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

Move to line 52?

Copy link
Member Author

Choose a reason for hiding this comment

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

Same


/// @notice Returns the owner of the contract
/// @return address of the owner
function owner() external view returns (address) {
return _owner;
}

/// @notice Sets the owner of the contract
/// @param newOwner address of the new owner
function transferOwnership(address newOwner) external {
require(hasRole(DEFAULT_ADMIN_ROLE, _msgSender()), "Asset: Unauthorized");
Copy link
Contributor

Choose a reason for hiding this comment

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

same

_owner = newOwner;
emit OwnershipTransferred(_owner, newOwner);
}

uint256[48] private __gap;
}
1 change: 1 addition & 0 deletions packages/asset/contracts/interfaces/IAsset.sol
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ interface IAsset {
}

event TrustedForwarderChanged(address indexed newTrustedForwarderAddress);
event OwnershipTransferred(address indexed previousOwner, address indexed newOwner);

/// @notice Mint new tokens
/// @dev Only callable by the minter role
Expand Down
1 change: 1 addition & 0 deletions packages/asset/contracts/interfaces/ICatalyst.sol
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ interface ICatalyst {
event DefaultRoyaltyChanged(address indexed newDefaultRoyaltyRecipient, uint256 newDefaultRoyaltyAmount);
event BaseURISet(string baseURI);
event OperatorRegistrySet(address indexed registry);
event OwnershipTransferred(address indexed previousOwner, address indexed newOwner);

/// @notice Mints a new token, limited to MINTER_ROLE only
/// @param to The address that will own the minted token
Expand Down
28 changes: 28 additions & 0 deletions packages/asset/test/Asset.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,34 @@ describe('Base Asset Contract (/packages/asset/contracts/Asset.sol)', function (
expect(await AssetContract.symbol()).to.be.equal('ASSET');
});
});
describe('Owner', function () {
it('should not have the owner set when initially deployed', async function () {
const {AssetContract} = await runAssetSetup();
expect(await AssetContract.owner()).to.be.equal(
ethers.constants.AddressZero
);
});
it('allows DEFAULT_ADMIN_ROLE to transfer the ownership', async function () {
const {AssetContract, assetAdmin} = await runAssetSetup();
await AssetContract.connect(assetAdmin).transferOwnership(
assetAdmin.address
);
expect(await AssetContract.owner()).to.be.equals(assetAdmin.address);
});
it('does not allow non-DEFAULT_ADMIN_ROLE account to transfer the ownership', async function () {
const {AssetContract, deployer} = await runAssetSetup();
await expect(
AssetContract.connect(deployer).transferOwnership(deployer.address)
).to.be.revertedWith('Asset: Unauthorized');
});
it('emits OwnershipTransferred event when DEFAULT_ADMIN_ROLE transfers the ownership', async function () {
const {AssetContract, assetAdmin} = await runAssetSetup();
const tx = await AssetContract.connect(assetAdmin).transferOwnership(
assetAdmin.address
);
await expect(tx).to.emit(AssetContract, 'OwnershipTransferred');
});
});
describe('Access Control', function () {
it('should have MINTER_ROLE defined', async function () {
const {AssetContract} = await runAssetSetup();
Expand Down
26 changes: 26 additions & 0 deletions packages/asset/test/Catalyst.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,32 @@ describe('Catalyst (/packages/asset/contracts/Catalyst.sol)', function () {
).to.revertedWith('Catalyst: CID cant be empty');
});
});
describe('Owner', function () {
it('should not have the owner set when initially deployed', async function () {
const {catalyst} = await runCatalystSetup();
expect(await catalyst.owner()).to.be.equal(ethers.constants.AddressZero);
});
it('allows DEFAULT_ADMIN_ROLE to transfer the ownership', async function () {
const {catalyst, catalystAdmin} = await runCatalystSetup();
await catalyst
.connect(catalystAdmin)
.transferOwnership(catalystAdmin.address);
expect(await catalyst.owner()).to.be.equals(catalystAdmin.address);
});
it('does not allow non-DEFAULT_ADMIN_ROLE account to transfer the ownership', async function () {
const {catalyst, deployer} = await runCatalystSetup();
await expect(
catalyst.connect(deployer).transferOwnership(deployer.address)
).to.be.revertedWith('Asset: Unauthorized');
});
it('emits OwnershipTransferred event when DEFAULT_ADMIN_ROLE transfers the ownership', async function () {
const {catalyst, catalystAdmin} = await runCatalystSetup();
const tx = await catalyst
.connect(catalystAdmin)
.transferOwnership(catalystAdmin.address);
await expect(tx).to.emit(catalyst, 'OwnershipTransferred');
});
});
describe('Admin Role', function () {
it('Admin can set minter', async function () {
const {catalystAsAdmin, user1, minterRole} = await runCatalystSetup();
Expand Down
2 changes: 2 additions & 0 deletions packages/asset/test/fixtures/asset/assetFixture.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ export function generateAssetId(

export async function runAssetSetup() {
const [
deployer,
assetAdmin,
owner,
secondOwner,
Expand Down Expand Up @@ -272,6 +273,7 @@ export async function runAssetSetup() {
AssetContractAsMinter,
AssetContractAsBurner,
AssetContractAsAdmin,
deployer,
owner,
assetAdmin,
minter,
Expand Down
1 change: 1 addition & 0 deletions packages/deploy/.gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,4 @@ generated-markups

# editors
.idea
deployments/localhost
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ const func: DeployFunction = async function (
await deploy('Catalyst', {
from: deployer,
log: true,
contract: '@sandbox-smart-contracts/asset/contracts/Catalyst.sol:Catalyst',
contract:
'@sandbox-smart-contracts/asset-1.0.3/contracts/Catalyst.sol:Catalyst',
proxy: {
owner: upgradeAdmin,
proxyContract: 'OpenZeppelinTransparentProxy',
Expand Down
35 changes: 35 additions & 0 deletions packages/deploy/deploy/300_catalyst/303_catalyst_upgrade-v2.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import {DeployFunction} from 'hardhat-deploy/types';
import {HardhatRuntimeEnvironment} from 'hardhat/types';

const func: DeployFunction = async function (
hre: HardhatRuntimeEnvironment
): Promise<void> {
const {deployments, getNamedAccounts, ethers} = hre;
const {execute, read, catchUnknownSigner, deploy} = deployments;
const {deployer, upgradeAdmin, catalystAdmin} = await getNamedAccounts();

await deploy('Catalyst', {
from: deployer,
log: true,
contract: '@sandbox-smart-contracts/asset/contracts/Catalyst.sol:Catalyst',
proxy: {
owner: upgradeAdmin,
proxyContract: 'OpenZeppelinTransparentProxy',
upgradeIndex: 1,
},
});

if ((await read('Catalyst', 'owner')) === ethers.constants.AddressZero) {
await catchUnknownSigner(
execute(
'Catalyst',
{from: catalystAdmin, log: true},
'transferOwnership',
catalystAdmin
)
);
}
};
Comment on lines +22 to +32
Copy link
Contributor

Choose a reason for hiding this comment

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

is better to split this in a separated step, because if something goes wrong it is easier to re-run it separately

export default func;
func.tags = ['Catalyst_upgrade'];
func.dependencies = ['Catalyst_deploy', 'Catalyst_setup'];
2 changes: 1 addition & 1 deletion packages/deploy/deploy/400_asset/402_deploy_asset.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ const func: DeployFunction = async function (hre: HardhatRuntimeEnvironment) {

await deploy('Asset', {
from: deployer,
contract: '@sandbox-smart-contracts/asset/contracts/Asset.sol:Asset',
contract: '@sandbox-smart-contracts/asset-1.0.3/contracts/Asset.sol:Asset',
proxy: {
owner: upgradeAdmin,
proxyContract: 'OpenZeppelinTransparentProxy',
Expand Down
34 changes: 34 additions & 0 deletions packages/deploy/deploy/400_asset/408_asset_upgrade-v2.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import {HardhatRuntimeEnvironment} from 'hardhat/types';
import {DeployFunction} from 'hardhat-deploy/types';

const func: DeployFunction = async function (hre: HardhatRuntimeEnvironment) {
const {deployments, getNamedAccounts, ethers} = hre;
const {read, catchUnknownSigner, execute, deploy} = deployments;
const {deployer, upgradeAdmin, assetAdmin} = await getNamedAccounts();

await deploy('Asset', {
from: deployer,
contract: '@sandbox-smart-contracts/asset/contracts/Asset.sol:Asset',
proxy: {
owner: upgradeAdmin,
proxyContract: 'OpenZeppelinTransparentProxy',
upgradeIndex: 1,
},
log: true,
});

if ((await read('Asset', 'owner')) === ethers.constants.AddressZero) {
await catchUnknownSigner(
execute(
'Asset',
{from: assetAdmin, log: true},
'transferOwnership',
assetAdmin
)
);
}
};
Comment on lines +20 to +30
Copy link
Contributor

Choose a reason for hiding this comment

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

is better to split this in a separated step, because if something goes wrong it is easier to re-run it separately

Copy link
Member Author

Choose a reason for hiding this comment

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

If the deployment of the upgrade fails this won't be called, and its a function that we likely call only once during the upgrade process.

I feel like its integral to this upgrade.

export default func;

func.tags = ['Asset_upgrade'];
func.dependencies = ['Asset_deploy', 'Asset_setup'];
4 changes: 4 additions & 0 deletions packages/deploy/hardhat.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@ import './tasks/importedPackages';
// Package name : solidity source code path
const importedPackages = {
'@sandbox-smart-contracts/asset': 'contracts/',
'@sandbox-smart-contracts/asset-1.0.3': [
'contracts/Asset.sol',
'contracts/Catalyst.sol',
],
'@sandbox-smart-contracts/giveaway': 'contracts/SignedMultiGiveaway.sol',
'@sandbox-smart-contracts/faucets': 'contracts/FaucetsERC1155.sol',
'@sandbox-smart-contracts/marketplace': [
Expand Down
1 change: 1 addition & 0 deletions packages/deploy/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
"private": true,
"dependencies": {
"@sandbox-smart-contracts/asset": "*",
"@sandbox-smart-contracts/asset-1.0.3": "npm:@sandbox-smart-contracts/asset@1.0.3",
"@sandbox-smart-contracts/core": "*",
"@sandbox-smart-contracts/dependency-operator-filter": "*",
"@sandbox-smart-contracts/dependency-royalty-management": "*",
Expand Down
7 changes: 5 additions & 2 deletions packages/deploy/test/asset/AssetCreate.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,18 @@ const setupTest = deployments.createFixture(
await getNamedAccounts();
await deployments.fixture();
const Asset = await deployments.get('Asset');
const AssetContract = await ethers.getContractAt('Asset', Asset.address);
const AssetContract = await ethers.getContractAt(
'@sandbox-smart-contracts/asset/contracts/Asset.sol:Asset',
Asset.address
);
const AssetCreate = await deployments.get('AssetCreate');
const AssetCreateContract = await ethers.getContractAt(
'AssetCreate',
AssetCreate.address
);
const Catalyst = await deployments.get('Catalyst');
const CatalystContract = await ethers.getContractAt(
'Catalyst',
'@sandbox-smart-contracts/asset/contracts/Catalyst.sol:Catalyst',
Catalyst.address
);
const TRUSTED_FORWARDER = await deployments.get('TRUSTED_FORWARDER_V2');
Expand Down
7 changes: 5 additions & 2 deletions packages/deploy/test/asset/AssetReveal.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,18 @@ const setupTest = deployments.createFixture(
await getNamedAccounts();
await deployments.fixture('Asset');
const Asset = await deployments.get('Asset');
const AssetContract = await ethers.getContractAt('Asset', Asset.address);
const AssetContract = await ethers.getContractAt(
'@sandbox-smart-contracts/asset/contracts/Asset.sol:Asset',
Asset.address
);
const AssetReveal = await deployments.get('AssetReveal');
const AssetRevealContract = await ethers.getContractAt(
'AssetReveal',
AssetReveal.address
);
const Catalyst = await deployments.get('Catalyst');
const CatalystContract = await ethers.getContractAt(
'Catalyst',
'@sandbox-smart-contracts/asset/contracts/Catalyst.sol:Catalyst',
Catalyst.address
);
const TRUSTED_FORWARDER = await deployments.get('TRUSTED_FORWARDER_V2');
Expand Down
16 changes: 16 additions & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -2154,6 +2154,21 @@ __metadata:
languageName: node
linkType: hard

"@sandbox-smart-contracts/asset-1.0.3@npm:@sandbox-smart-contracts/asset@1.0.3":
version: 1.0.3
resolution: "@sandbox-smart-contracts/asset@npm:1.0.3"
dependencies:
"@manifoldxyz/libraries-solidity": ^1.0.4
"@manifoldxyz/royalty-registry-solidity": ^2.0.3
"@openzeppelin/contracts": 4.9.3
"@openzeppelin/contracts-upgradeable": 4.9.3
"@sandbox-smart-contracts/dependency-metatx": 1.0.1
"@sandbox-smart-contracts/dependency-operator-filter": 1.0.1
"@sandbox-smart-contracts/dependency-royalty-management": 1.0.2
checksum: bbf63b91b98bf694fd68592d7f35a5aacafb2bfd67243352f27b4bf062dc17c13509c480571077cadc451059c15587029145caece2814495d69cd5241e82f6db
languageName: node
linkType: hard

"@sandbox-smart-contracts/asset@*, @sandbox-smart-contracts/asset@workspace:packages/asset":
version: 0.0.0-use.local
resolution: "@sandbox-smart-contracts/asset@workspace:packages/asset"
Expand Down Expand Up @@ -2411,6 +2426,7 @@ __metadata:
"@nomiclabs/hardhat-ethers": ^2.2.3
"@openzeppelin/hardhat-upgrades": ^2.5.0
"@sandbox-smart-contracts/asset": "*"
"@sandbox-smart-contracts/asset-1.0.3": "npm:@sandbox-smart-contracts/asset@1.0.3"
"@sandbox-smart-contracts/core": "*"
"@sandbox-smart-contracts/dependency-operator-filter": "*"
"@sandbox-smart-contracts/dependency-royalty-management": "*"
Expand Down
Loading