-
Notifications
You must be signed in to change notification settings - Fork 60
feat: implement IFT token standard in Solidity #864
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
base: main
Are you sure you want to change the base?
Conversation
| /// @param amount The amount of tokens involved in the pending transfer | ||
| struct PendingTransfer { | ||
| address sender; | ||
| uint256 amount; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: clientId and sequence are mapping keys, hence they are not included in the struct.
| /// @param denom_ The denom of the token on the Cosmos SDK chain (e.g., "uatom", "ibc/...") | ||
| /// @param icaAddress_ The interchain account address on the Cosmos SDK chain | ||
| constructor(string memory bridgeReceiveTypeUrl_, string memory denom_, string memory icaAddress_) { | ||
| bridgeReceiveTypeUrl = bridgeReceiveTypeUrl_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: I made it configurable instead of hardcoding.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #864 +/- ##
=======================================
Coverage 99.89% 99.90%
=======================================
Files 22 26 +4
Lines 982 1079 +97
=======================================
+ Hits 981 1078 +97
Misses 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
contracts/IFTBase.sol
Outdated
| /// @notice Initializes the IFT base contract | ||
| /// @param ics27Gmp_ The ICS27-GMP contract address | ||
| /// @param authority_ The AccessManager contract address for access control | ||
| constructor(IICS27GMP ics27Gmp_, address authority_) AccessManaged(authority_) { | ||
| IFTStorage storage $ = _getIFTStorage(); | ||
| $._ics27Gmp = ics27Gmp_; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The storage slot implies this is an upgradable contract, so we probably should use an initializer instead of constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need to include new restricted functions on our deploy scripts
| { | ||
| require(bytes(clientId).length > 0, IFTEmptyClientId()); | ||
| require(bytes(counterpartyIFTAddress).length > 0, IFTEmptyCounterpartyAddress()); | ||
| require(iftSendCallConstructor != address(0), IFTZeroAddressConstructor()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may consider checking interface with ERC 165
contracts/IFTBase.sol
Outdated
| /// @title IFT Base Contract | ||
| /// @notice Abstract base contract for Interchain Fungible Tokens | ||
| /// @dev Extend this contract and implement the ERC20 constructor to create an IFT token | ||
| abstract contract IFTBase is IIFTErrors, IIFT, ERC20, AccessManaged, IBCCallbackReceiver { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should add a warning somewhere that this is experimental. Similar to the warning on attestor LC in ibc-go
| require(bytes(clientId).length > 0, IFTEmptyClientId()); | ||
| require(bytes(receiver).length > 0, IFTEmptyReceiver()); | ||
| require(amount > 0, IFTZeroAmount()); | ||
| require(timeoutTimestamp > block.timestamp, IFTTimeoutInPast(timeoutTimestamp, uint64(block.timestamp))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is validated by the router, but I'll double check
|
|
||
| IFTBaseStorage storage $ = _getIFTBaseStorage(); | ||
| IIFTMsgs.IFTBridge memory bridge = $._iftBridges[clientId]; | ||
| require(keccak256(bytes(bridge.clientId)) == keccak256(bytes(clientId)), IFTBridgeNotFound(clientId)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The keccak256 comparison here is redundant since bridge is already looked up using clientId as the mapping key. If the mapping returns a populated struct, bridge.clientId will equal clientId by definition.
| require(keccak256(bytes(bridge.clientId)) == keccak256(bytes(clientId)), IFTBridgeNotFound(clientId)); | |
| require(bytes(bridge.clientId).length > 0, IFTBridgeNotFound(clientId)); |
…xtend test coverage (#874)
Description
closes: STACK-1906
Implements the Interchain Fungible Token (IFT) standard in Solidity, enabling cross-chain token transfers via ICS27-GMP.
IFTBaseabstract contract that extends ERC20 with cross-chain transfer capabilitiesEVMIFTSendCallConstructorfor EVM-to-EVM bridgesAdd(will be added together with Cosmos SDK IFT implementation)CosmosIFTSendCallConstructorfor EVM-to-Cosmos bridgesBefore we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.