feat: custom token registrars #116
Conversation
# Conflicts: # test/TokenService.js
| function getCanonicalTokenSalt(address tokenAddress) external view returns (bytes32 salt); | ||
|
|
||
| function getCanonicalTokenId(address tokenAddress) external view returns (bytes32 tokenId); |
There was a problem hiding this comment.
remove get prefix from registrar contracts as well (along with ITS)
|
|
||
| function chainNameHash() external view returns (bytes32); | ||
|
|
||
| function getStandardizedTokenSalt(address deployer, bytes32 salt) external view returns (bytes32); |
| bytes32 internal constant PREFIX_CANONICAL_TOKEN_SALT = keccak256('canonical-token-salt'); | ||
| bytes32 private constant CONTRACT_ID = keccak256('canonical-token-registrar'); | ||
|
|
||
| constructor(address interchainTokenServiceAddress) { |
There was a problem hiding this comment.
| constructor(address interchainTokenServiceAddress) { | |
| constructor(address interchainTokenService) { |
address is redundant
| } | ||
|
|
||
| function deployAndRegisterRemoteCanonicalToken(bytes32 salt, string calldata destinationChain, uint256 gasValue) external payable { | ||
| // This ensures that the token manages has been deployed by this address, so it's safe to trust it. |
There was a problem hiding this comment.
| // This ensures that the token manages has been deployed by this address, so it's safe to trust it. | |
| // This ensures that the token manager has been deployed by this address, so it's safe to trust it. |
| tokenSymbol, | ||
| tokenDecimals, | ||
| distributor, | ||
| '', |
There was a problem hiding this comment.
mintTo isn't being provided so I think the initial mint will be skipped? Can you add that, and add test coverage for it?
| token.safeTransfer(sender, mintAmount); | ||
| } | ||
|
|
||
| function deployRemoteStandarizedToken( |
There was a problem hiding this comment.
Can we combine the two deploy methods into one? The interchain address tracker has the current chain, so we can specialize the logic by comparing the chain name being passed. Reduces number of entrypoints. Perhaps we can simplify this in ITS itself.
There was a problem hiding this comment.
Similarly, we could combine deploy custom token manager current/remote into one method
| string calldata symbol, | ||
| uint8 decimals, | ||
| uint256 mintAmount, | ||
| address distributor |
There was a problem hiding this comment.
let operator still be different than sender, since it's an argument for the remote deploy method as well?
| if (!token.isDistributor(additionalDistributor)) revert NotDistributor(additionalDistributor); | ||
| distributor = additionalDistributor.toBytes(); | ||
| } else if (mintAmount != 0) { | ||
| revert NonZeroMintAmount(); |
There was a problem hiding this comment.
add a comment that if distributor is not set than mintAmount must be 0 to enforce that additional token deployments can't change it
| } else if (mintAmount != 0) { | ||
| revert NonZeroMintAmount(); | ||
| } | ||
| if (optionalOperator != address(0)) { |
There was a problem hiding this comment.
newline before this statement for readability
| // This ensures that the token manager has been deployed by this address, so it's safe to trust it. | ||
| bytes32 tokenId = getCanonicalTokenId(tokenAddress); | ||
| // slither-disable-next-line unused-return | ||
| service.getValidTokenManagerAddress(tokenId); |
There was a problem hiding this comment.
interchainTransfer will fail if it's not valid so is this check needed?
| struct DeployParams { | ||
| address deployer; | ||
| bytes distributor; | ||
| bytes operator; | ||
| } | ||
|
|
||
| mapping(bytes32 => DeployParams) public deploymentParameterMap; |
|
|
||
| import { AddressBytesUtils } from '../libraries/AddressBytesUtils.sol'; | ||
|
|
||
| contract StandardizedTokenRegistrar is IStandardizedTokenRegistrar, ITokenManagerType, Multicall, Upgradable { |
There was a problem hiding this comment.
Can we combine the two registrars into one? Easier to manage operations around it and for UI to integrate
| function registerCanonicalToken(address tokenAddress) external payable returns (bytes32 tokenId) { | ||
| bytes memory params = abi.encode('', tokenAddress); | ||
| bytes32 salt = getCanonicalTokenSalt(tokenAddress); | ||
| tokenId = service.deployCustomTokenManager(salt, TokenManagerType.LOCK_UNLOCK, params); |
There was a problem hiding this comment.
Shall we remove the canonical register/deploy methods from ITS?
| ); | ||
| } | ||
|
|
||
| function transferCanonicalToken( |
There was a problem hiding this comment.
If we can figure out how to make this generic for other token types, then we could drop the initial mint amount from the official ITS deploy payload spec altogether.
Perhaps this approach could work:
- The deploy method for the current chain takes in mintTo/mintAmount argument.
- The UI sets this mintTo address to be the registrar contract.
- The deploy remote method doesn't forward mintAmount anymore. It just deploys.
- The subsequent transfer calls in the multicall then call interchainTransfer method (with an approval to the token manager if necessary).
This way the user doesn't need to pre-approve a non-existent token. Deploy and transfer are independent methods. It can also remove the concern of arbitrary mints on future deployments at the protocol level.
AXE-2000
Token registrars help guarantee the integrity of token deployments (token decimals, mint amount etc.) to additional chains via ITS in the future