feat: make token manager the minter for interchain tokens #313
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #313 +/- ##
==========================================
- Coverage 99.44% 99.44% -0.01%
==========================================
Files 20 20
Lines 724 722 -2
Branches 170 171 +1
==========================================
- Hits 720 718 -2
Misses 4 4 ☔ View full report in Codecov by Sentry. |
contracts/InterchainTokenFactory.sol
Outdated
| @@ -149,6 +149,8 @@ contract InterchainTokenFactory is IInterchainTokenFactory, ITokenManagerType, M | |||
| if (minter == address(interchainTokenService)) revert InvalidMinter(minter); | |||
|
|
|||
| minterBytes = minter.toBytes(); | |||
| } else { | |||
| revert EmptyInterchainToken(); | |||
There was a problem hiding this comment.
wrong base branch?
Can you create the PR on top of main instead of another branch, so they can be merged in any order?
| @@ -97,7 +98,7 @@ contract InterchainToken is InterchainTokenStandard, ERC20Permit, Minter, IInter | |||
| * Also add the provided address as a minter. If `address(0)` was provided, | |||
| * add it as a minter to allow anyone to easily check that no custom minter was set. | |||
| */ | |||
| _addMinter(interchainTokenService_); | |||
| _addMinter(IInterchainTokenService(interchainTokenService_).tokenManagerAddress(tokenId_)); | |||
There was a problem hiding this comment.
feels like a weird re-entrancy to call ITS back during deployment. better to pass in the token manager, or have ITS call transferMintership after deployment. The latter avoids needing to change the token bytecode, and keeps the logic isolated in ITS
| interchainToken = await deployContract(owner, 'InterchainToken', [owner.address]); | ||
| interchainTokenDeployer = await deployContract(owner, 'InterchainTokenDeployer', [interchainToken.address]); | ||
|
|
||
| ({ interchainTokenDeployer } = await deployAll(owner, 'Test')); |
There was a problem hiding this comment.
if you make the previous suggested change, then you don't need to deploy all contracts for this to work
| const name = 'tokenName'; | ||
| const symbol = 'tokenSymbol'; | ||
| const decimals = 18; | ||
| const MINTER_ROLE = 0; | ||
|
|
||
| before(async () => { | ||
| interchainToken = await deployContract(ownerWallet, 'InterchainToken', [service]); | ||
| interchainTokenDeployer = await deployContract(ownerWallet, 'InterchainTokenDeployer', [interchainToken.address]); | ||
| ({ interchainTokenDeployer, service } = await deployAll(ownerWallet, 'Test')); |
| @@ -334,7 +334,7 @@ contract InterchainTokenFactory is IInterchainTokenFactory, ITokenManagerType, M | |||
| if (!token.isMinter(minter)) revert NotMinter(minter); | |||
There was a problem hiding this comment.
add changeset and briefly discuss the behaviour change reason
| @@ -334,7 +334,7 @@ contract InterchainTokenFactory is IInterchainTokenFactory, ITokenManagerType, M | |||
| if (!token.isMinter(minter)) revert NotMinter(minter); | |||
|
|
|||
| // Sanity check to prevent accidental use of the current ITS address as the token minter | |||
There was a problem hiding this comment.
update DESIGN.md for this behaviour change
No description provided.