feat: add ICS20 App on IBC #793

sdpisreddevil · 2023-12-13T02:55:59Z

Description

Commit Message

type: commit message

see the guidelines for commit messages.

Changelog Entry

version: <log entry>

Checklist

I have performed a self-review of my own code
I have documented my code in accordance with the documentation guidelines
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
I have run the unit tests
I only have one commit (if not, squash them into one commit).
I have a descriptive commit message that adheres to the commit message guidelines
I have added version bump label on PR.

Please review the CONTRIBUTING.md file for detailed contributing guidelines.

added the required files

contracts/javascore/ibc/src/main/java/ibc/ics20/app/ICS20Bank.java

ibrizsabin · 2023-12-13T07:43:37Z

contracts/javascore/ibc/src/main/java/ibc/ics20/app/ICS20Transfer.java

+        byte[] denomPrefix = getDenomPrefix(packet.data.sourcePort, packet.data.sourceChannel);
+        byte[] denom = packet.data.denom.getBytes();
+        if (denom.length >= denomPrefix.length) {
+//            denom slicing todo


wouldnot length of denom be always greater than denomPrefix? .i think we would need to compare denom prefix with packet sourcePort and sourceChannel to determine if sender is source or sink then burn token if sender is source.

contracts/javascore/ibc/src/main/java/ibc/ics20/app/ICS20Transfer.java

ibrizsabin · 2023-12-13T07:45:44Z

contracts/javascore/ibc/src/main/java/ibc/ics20/app/ICS20Transfer.java

+    @External
+    public void onAcknowledgementPacket(byte[] calldata, byte[] acknowledgement, Address relayer) {
+        Context.println("onAcknowledgementPacket");
+//        Context.require(Context.getCaller().equals(getIBCAddress()), "caller is not handler");


we can decode packet from calldata ... and lets rename it to packetBytes as well.

ibrizsabin · 2023-12-13T07:47:03Z

contracts/javascore/ibc/src/main/java/ibc/ics20/app/ICS20Transfer.java

+    }
+
+    @External
+    public void onTimeoutPacket(Packet packet, byte[] proofHeight, byte[] proof, BigInteger nextSequenceRecv) {


we need to refund on packet timeout as well.

contracts/javascore/ibc/src/main/java/ibc/ics20/app/ICS20Transfer.java

ibrizsabin · 2023-12-13T07:54:49Z

contracts/javascore/ibc/src/main/java/ibc/ics20/app/ICS20TransferBank.java

+        byte[] packetData = ICS20Lib.marshalJson(denom,amount, ICS20Transfer._encodeSender(caller),receiver);
+
+
+//        ibcHandler.sendPacket(sourcePort, sourceChannel, Height., timeoutHeight);


i think we can merge this with ICS20Transfer and treat it as the entry contract. Is there any reason to separate this out?

- ics20App transfer (denom encoding) - remove unused methods/variables

- ics20App transfer (denom encoding) - add ics20Bank address getter - remove unused methods

contracts/javascore/ibc/src/main/java/ibc/ics20/app/ICS20Bank.java

…nsfer Fix/build separate ics20 transfer

AntonAndell · 2024-02-12T12:32:59Z

@nightowl121 Should we mabye have the bank implment IRC31? where a denom gets assigned a ID? At the current state these tokens cannot be used by contracts at all.

AntonAndell · 2024-02-14T07:14:14Z

contracts/javascore/modules/ics20app/src/main/java/ibc/ics20app/ICS20Transfer.java

+        Context.require(version.equals(ICS20_VERSION), "version should be same with ICS20_VERSION");
+        Channel.Counterparty counterparty = Channel.Counterparty.decode(counterpartyPb);
+        destinationPort.set(channelId, counterparty.getPortId());
+        channelEscrowAddresses.set(channelId, Context.getAddress());


Could this logic not be completely omitted, is there any scenario where escrow won't be this contract?

AntonAndell · 2024-02-14T07:16:01Z

contracts/javascore/modules/ics20app/src/main/java/ibc/ics20app/ICS20Transfer.java

+        if (denom.length >= denomPrefix.length && Ops.hasPrefix(denom, denomPrefix)) {
+            Context.require(_mint(Address.fromString(data.sender), data.denom, data.amount), "ICS20: mint failed");
+        } else {
+            Context.require(_transferFrom(getEscrowAddress(sourceChannel), Address.fromString(data.sender), data.denom, data.amount), "ICS20: transfer failed");


Remove require on _mint and _transfer from and convert those functions to void

AntonAndell · 2024-02-14T07:17:13Z

contracts/javascore/modules/ics20app/src/main/java/ibc/ics20app/ICS20Transfer.java

+    boolean _burn(Address account, String denom, BigInteger amount) {
+        Context.call(bank.get(), "burn", account, denom, amount);
+        return true;
+    }


AntonAndell · 2024-02-14T07:17:19Z

contracts/javascore/modules/ics20app/src/main/java/ibc/ics20app/ICS20Transfer.java

+    boolean _transferICX(Address receiver, BigInteger amount) {
+        Context.require(Context.getBalance(Context.getAddress()).compareTo(amount) >= 0, "ICS20App: insufficient balance for transfer");
+        Context.transfer(receiver, amount);
+        return true;
+    }


AntonAndell · 2024-02-14T07:20:28Z

contracts/javascore/modules/ics20app/src/main/java/ibc/ics20app/ICS20Transfer.java

+    protected static boolean _decodeReceiver(String receiver) {
+        boolean flag;
+        try {
+            Address.fromString(receiver);
+            flag = true;
+        } catch (Exception e) {
+            flag = false;
+
+        }
+        return flag;
+    }


Suggested change

protected static boolean _decodeReceiver(String receiver) {

boolean flag;

try {

Address.fromString(receiver);

flag = true;

} catch (Exception e) {

flag = false;

}

return flag;

}

protected static boolean _decodeReceiver(String receiver) {

try {

Address.fromString(receiver);

flag = true;

} catch (Exception e) {

return false;

}

}

AntonAndell · 2024-02-14T07:34:28Z

contracts/javascore/modules/ics20app/src/main/java/ibc/ics20app/ICS20Transfer.java

+    }
+
+    @External
+    public byte[] onRecvPacket(byte[] packet, Address relayer) {


Since this fucntion is not allowed to fail, perhaps create a internal function and wrap this in a try catch to always return failed ack in case of failure

AntonAndell · 2024-02-14T07:36:28Z

contracts/javascore/modules/ics20app/src/main/java/ibc/ics20app/ICS20TransferBank.java

+        Address caller = Context.getCaller();
+        if (denom.equals("icx")) {
+            Context.require(Context.getValue().compareTo(BigInteger.ZERO) > 0, "ICS20App: icx transfer failed");
+            Context.require(Context.getValue().compareTo(amount) == 0, "ICS20App: icx value is not equal to amount sent");


user .equals

AntonAndell · 2024-02-14T07:37:56Z

contracts/javascore/modules/ics20app/src/main/java/ibc/ics20app/ICS20TransferBank.java

+                Context.require(_transferFrom(caller, ICS20Transfer.getEscrowAddress(sourceChannel), denom, amount), "ICS20App: transfer failed");
+            } else {
+                Context.require(_burn(caller, denom, amount), "ICS20App: Burn failed");


Remove require since the wont work

AntonAndell · 2024-02-14T07:38:43Z

contracts/javascore/modules/ics20app/src/main/java/ibc/ics20app/ICS20TransferBank.java

+            byte[] denomPrefix = ICS20Transfer.getDenomPrefix(sourcePort, sourceChannel);
+            String denomText = new String(denomPrefix);
+            if (!denom.startsWith(denomText)) {
+                Context.require(_transferFrom(caller, ICS20Transfer.getEscrowAddress(sourceChannel), denom, amount), "ICS20App: transfer failed");


When is this case used on ICON?

AntonAndell · 2024-02-14T07:39:50Z

contracts/javascore/modules/ics20bank/src/main/java/ibc/ics20bank/ICS20Bank.java

+    @External
+    public void deposit(Address tokenContract, BigInteger amount, Address receiver) {
+        Context.require(tokenContract.isContract(), TAG + ": tokenContract is not a contract");
+        Context.call(tokenContract, "transferFrom", Context.getCaller(), Context.getAddress(), amount);
+        _mint(receiver, tokenContract.toString(), amount);
+    }
+


What tokens are these?

To support ICON tokens it should be done via tokenFallback

AntonAndell · 2024-02-14T07:48:37Z

I am not sure what to do about the token standard though, if we want IRC31 or go with a IRC2 standard so that tokens can be used as normal tokens.

nightowl121 · 2024-02-15T09:18:52Z

@nightowl121 Should we mabye have the bank implment IRC31? where a denom gets assigned a ID? At the current state these tokens cannot be used by contracts at all.

Yeah it would be helpful if we make it compatible with an existing standard or else we would have to propose a new standard to be used by the contracts.

izyak · 2024-03-14T05:55:21Z

Merge this PR to another branch to start working on this new branch for IRC2 support

sdpisreddevil added 6 commits November 28, 2023 17:35

initial commits

45e890d

added the required files

add ICS20Bank contract

4b606e3

add ICS20Bank contract

c790b28

add ICS20Lib contract

bd9181f

add ICS20TransferBank contract

c074ebe

add ICS20Transfer contract

750fa1e

sdpisreddevil self-assigned this Dec 13, 2023

sdpisreddevil requested review from redlarva, ibrizsabin, AntonAndell and nightowl121 as code owners December 13, 2023 02:56

sdpisreddevil marked this pull request as draft December 13, 2023 02:56

sdpisreddevil changed the title ~~Add ICS20 App on IBC~~ feat: add ICS20 App on IBC Dec 13, 2023