ICS20 ADR Proposal #1884

Wizdave97 · 2022-02-17T08:12:12Z

Closes: https://github.com/informalsystems/ibc-rs/issues/2090

Description

PR author checklist:

Added changelog entry, using unclog.
Added tests: integration (for Hermes) or unit/mock tests (for modules).
Linked to GitHub issue.
Updated code comments and documentation (e.g., docs/).

Reviewer checklist:

Reviewed Files changed in the GitHub PR explorer.
Manually tested (in case integration/unit/mock tests are absent).

hu55a1n1 · 2022-02-17T10:34:01Z

Thanks, @Wizdave97! 🙏 This is very helpful work! I am currently working on a draft PR (#1838) where I am trying to define the API for ICS26 modules. While this PR is being reviewed, please feel free to check that out.

hu55a1n1

Thanks again for this PR @Wizdave97! 🙏 I have added some comments (most of which are just nits 🙂) and observations. I think it would be better to revisit this after we merge #1838, because that will have a significant impact on the module's design.

hu55a1n1 · 2022-02-18T08:34:35Z

docs/architecture/adr-008-ics20-implementation.md

+        /// Full denomination path
+        trace_path: String,
+        /// Base denimination
+        base_denom: String


I think it would be nice to have a newtype for Domain. This will help us enforce some restrictions (e.g. '/' not permitted, etc.). Ideally, we would want this type to implement to impl Asref<str>, FromStr & Display.

pub struct Denom(String);

hu55a1n1 · 2022-02-18T08:38:19Z

docs/architecture/adr-008-ics20-implementation.md

+    }
+
+    /// This struct and it's implementations should help identifying denomination traces
+    pub struct DenomTrace {


DenomTrace could also be a modelled like so ->

struct TracePrefix { port_id: PortId, channel_id: ChannelId, } pub struct DenomTrace { trace_path: Vec<TracePrefix>, base_denom: Denom }

Might be relevant for the TracePrefix struct described above -> informalsystems/ibc-rs#745

hu55a1n1 · 2022-02-18T08:40:56Z

docs/architecture/adr-008-ics20-implementation.md

+
+    impl DenomTrace {
+        /// Creates a denom trace from a raw string
+        pub fn parse_denom_trace(raw_trace: &str) -> Self {


This method can fail as not all strings are valid DenomTraces. I would suggest we instead implement FromStr. And provide no other ctors.

hu55a1n1 · 2022-02-18T08:42:26Z

docs/architecture/adr-008-ics20-implementation.md

+            ...
+        }
+        /// Validates the denom trace fields
+        fn validate(&self) -> Result<(), ICS20Error> {


This wouldn't be required if the type is only constructible via FromStr.

hu55a1n1 · 2022-02-18T08:43:22Z

docs/architecture/adr-008-ics20-implementation.md

+        fn get_prefix(&self) -> String {
+            ...
+        }
+    }


A has_prefix() method would be useful (to determine if the current zone is source or sink).

hu55a1n1 · 2022-02-18T08:54:19Z

docs/architecture/adr-008-ics20-implementation.md

+        }
+    }
+
+    type Coin = ibc_proto::cosmos::base::v1beta1::Coin;


Would be nice to have a Coin domain type. cosmrs has a decent implementation -> https://github.com/cosmos/cosmos-rust/blob/main/cosmrs/src/base.rs#L118.

hu55a1n1 · 2022-02-18T09:05:30Z

docs/architecture/adr-008-ics20-implementation.md

+    /// acknowledgement written on the receiving chain. If the acknowledgement
+    /// was a success then nothing occurs. If the acknowledgement failed, then
+    /// the sender is refunded their tokens.
+    pub on_acknowledgement_packet<Ctx>(ctx: &Ctx, packet: Packet, data: MsgTransfer) -> Result<(), ICS20Error>


IIUC, this function should return an Acknowledgement type - both on success and failure. So we're also missing this Acknowledgement type. We'd also want this type to implement From<GenericAcknowledgement> or From<Vec<u8>>. See ICS20 technical specification ->

pub enum Acknowledgement { Success, // => b"AQ==" Error(String), }

Additionally, this type must also implement the Acknowledgement trait -> https://github.com/informalsystems/ibc-rs/pull/1838#discussion_r809215296

hu55a1n1 · 2022-02-18T09:09:47Z

docs/architecture/adr-008-ics20-implementation.md

+```rust
+    /// Initiates ICS20 token transfer
+    /// Escrows tokens from sender and registers the send packet 
+    pub send_transfer<Ctx>(ctx: &Ctx, source_port: PortId, msg: MsgTransfer) -> Result<(), ICS20Error>


The source_port should already be available from the MsgTransfer, right? So maybe we can remove the redundant arg.

hu55a1n1 · 2022-02-18T09:16:44Z

docs/architecture/adr-008-ics20-implementation.md

+        fn get_port(&self) -> Result<PortId, Error>;
+    }
+
+    pub trait BankKeeper<AccountId: Into<String>> {


hu55a1n1 · 2022-02-18T09:19:34Z

docs/architecture/adr-008-ics20-implementation.md

+
+The following handlers are recommended to be implemented in the ics20_fungible_token_transfer application in the ibc-rs crate
+
+These handlers will be executed in the module callbacks of any thirdparty IBCmodule that is implementing an ICS20 application on-chain


I think the design of these handlers will depend on the design of the ICS26 Module trait. We might need to impose additional restrictions due to trait object safety. https://github.com/informalsystems/ibc-rs/pull/1838/files#r809237272

Wizdave97 · 2022-02-18T13:55:40Z

Thanks again for this PR @Wizdave97! 🙏 I have added some comments (most of which are just nits 🙂) and observations. I think it would be better to revisit this after we merge #1838, because that will have a significant impact on the module's design.

Thank you for the review, I'll work on your comments over the weekend.

hu55a1n1 · 2022-02-21T11:21:07Z

docs/architecture/adr-008-ics20-implementation.md

+        fn authenticate_capability(&self, cap: Capability, name: &str) -> bool;
+        /// claim_capability allows the transfer module to claim a capability that IBC module
+        /// passes to it
+        fn claim_capability(&self, cap: Capability, name: &str) -> Result<(), ICS20Error>;


I think this keeper must provide functions to read/modify the channelEscrowAddresses mapping in the module state.

hu55a1n1 · 2022-02-21T11:23:23Z

docs/architecture/adr-008-ics20-implementation.md

+    }
+
+    /// Handles incoming packets with ICS20 data
+    pub on_recv_packet<Ctx>(ctx: &Ctx, packet: Packet, data: MsgTransfer) -> Result<(), ICS20Error>


We also need to define a PacketData type for this module with a deserialize impl, something like ->

struct FungibleTokenPacketData { denomination: Denom, amount: U256, sender: AccountId, receiver: AccountId, }

Wizdave97 · 2022-04-05T11:30:07Z

I'll look into this and update it before Friday this week since the router has been merged.

adizere · 2022-04-05T15:26:21Z

Thanks David, once this is marked ready for review we'll shift some eyes on it again.

Wizdave97 · 2022-04-07T16:06:15Z

The call back handlers are meant to be called inside whatever is implementing the Module trait

…/ics-20-keeper-proposal

docs/architecture/adr-008-ics20-implementation.md

plafer · 2022-04-13T19:41:12Z

docs/architecture/adr-008-ics20-implementation.md

+
+    }
+
+    pub trait ICS20Keeper: BankKeeper<Self::AccountId> 


This is a general comment about the use of *Keeper and *Reader convention. We typically have *Reader traits all take &self as first argument, and *Keeper traits take &mut self. Could you make sure that this convention is followed throughout the ADR?

I also noticed that some of the functions don't have a &mut self but should, such as set_port().

plafer · 2022-04-13T19:55:11Z

docs/architecture/adr-008-ics20-implementation.md

+    }
+
+    pub trait ICS20Keeper: BankKeeper<Self::AccountId> 
+        + AccountKeeper<Self::AccountId>


AccountKeeper is never defined

Up. Let's clarify why is AccountKeeper necessary here.

plafer · 2022-04-13T19:57:15Z

docs/architecture/adr-008-ics20-implementation.md

+        /// Returns if receiving is allowed in the module params
+        fn is_receive_enabled(&self) -> bool;
+        /// Set the params (send_enabled and receive_enabled) for the module
+        fn set_module_params(send_enabled: Option<bool>, receive_enabled: Option<bool>) -> Result<(), ICS20Error>;


&mut self missing

plafer · 2022-04-13T20:09:28Z

docs/architecture/adr-008-ics20-implementation.md

+
+    /// sender_chain_is_source returns false if the denomination originally came
+    /// from the receiving chain and true otherwise.
+    fn sender_chain_is_source(source_port: &PortId, source_channel: &ChannelId, denom: String) -> bool {


We should combine sender_chain_is_source() and receiver_chain_is_source() into one get_source_chain() -> SourceChain (names TBD, please don't use those necessarily), where

enum SourceChain { Sender, Receiver, }

This increases readability, as match statements will be used where both possibilities are made explicit, instead of being captured in an if/else statement.

plafer · 2022-04-13T20:15:28Z

docs/architecture/adr-008-ics20-implementation.md

+    ...
+}
+
+impl Acknowledgement for ICS20Acknowledgement {


What do Acknowledgement and GenericAcknowledgement refer to?

plafer · 2022-04-13T20:23:37Z

docs/architecture/adr-008-ics20-implementation.md

+        }
+
+        /// Returns the prefix for this trace
+        pub fn has_prefix(denom: String, prefix: String) -> bool {


denom and prefix should be &str. Same goes for all other functions in the ADR that take a String as input

plafer · 2022-04-13T20:25:36Z

docs/architecture/adr-008-ics20-implementation.md

+}
+
+/// Implements logic for refunding a sender on packet timeout or acknowledgement error
+pub fn refund_packet_token<Ctx>(ctx: &Ctx, data: FungibleTokenPacketData) -> Result<(), ICS20Error>


Should be data: &FungibleTokenPacketData. This also applies to most other arguments in the ADR that are passed by value (e.g. all the PortId and ChannelId in ICS20Keeper).

Please re-review

adizere

Took the liberty to address some minor inconsistencies & issues.

@plafer and @hu55a1n1 is there anything outstanding in your opinion? Please have a final look and help ensure this PR gets resolved. Thank you!

adizere · 2022-04-20T13:44:45Z

docs/architecture/adr-008-ics20-implementation.md

+    }
+
+    pub trait ICS20Keeper: BankKeeper<Self::AccountId> 
+        + AccountKeeper<Self::AccountId>


Up. Let's clarify why is AccountKeeper necessary here.

@hu55a1n1

Following sync code review with @hu55a1n1 and @plafer.

hu55a1n1

💮

plafer

Great work, thank you for this @Wizdave97!

@hu55a1n1

* create ics20 proposal * enforce account id to implement string * update proposal * updated description * updated proposal * add proposal to table of contents * updates * pass values by reference * Minor fixes * More nits. * Nites: Spaces for readability * Rename * Trimmed down content to its essentials. Following sync code review with @hu55a1n1 and @plafer. * Typo & date * Fix formatting * Use footnotes * Document types * Update trait definitions * Fix code warnings Co-authored-by: Adi Seredinschi <[email protected]> Co-authored-by: Shoaib Ahmed <[email protected]>

Wizdave97 mentioned this pull request Feb 17, 2022

Complete ICS20 ft-transfer implementation informalsystems/ibc-rs#59

Closed

9 tasks

hu55a1n1 previously requested changes Feb 18, 2022

View reviewed changes

hu55a1n1 reviewed Feb 21, 2022

View reviewed changes

hu55a1n1 mentioned this pull request Mar 22, 2022

Complete ICS20 Implementation #1989

Merged

7 tasks

Wizdave97 added 5 commits April 6, 2022 13:41

create ics20 proposal

13c8b52

enforce account id to implement string

ec66015

update proposal

debddeb

updated description

b4573d7

updated proposal

71de87f

Wizdave97 force-pushed the david/ics-20-keeper-proposal branch from 7a57e18 to 71de87f Compare April 7, 2022 15:41

Wizdave97 marked this pull request as ready for review April 7, 2022 16:06

Wizdave97 requested review from adizere, ancazamfir and romac as code owners April 7, 2022 16:06

Wizdave97 requested a review from hu55a1n1 April 7, 2022 16:12

Merge branch 'master' of github.com:informalsystems/ibc-rs into david…

6188205

…/ics-20-keeper-proposal

adizere assigned plafer Apr 11, 2022

adizere changed the title ~~ICS20 Implementation Proposal~~ ICS20 ADR Proposal Apr 11, 2022

adizere reviewed Apr 12, 2022

View reviewed changes

docs/architecture/adr-008-ics20-implementation.md Outdated Show resolved Hide resolved

add proposal to table of contents

909ee4b

plafer reviewed Apr 13, 2022

View reviewed changes

Wizdave97 added 2 commits April 15, 2022 11:06

updates

ed37d6d

pass values by reference

fde6165

adizere added 3 commits April 20, 2022 15:31

Minor fixes

6d8e239

More nits.

1e79dd3

Nites: Spaces for readability

45932bb

adizere approved these changes Apr 20, 2022

View reviewed changes

adizere and others added 8 commits April 21, 2022 17:30

Rename

4f43aae

Trimmed down content to its essentials.

6792a6e

Following sync code review with @hu55a1n1 and @plafer.

Typo & date

2cadec6

Fix formatting

4e6a081

Use footnotes

25097f6

Document types

d22050c

Update trait definitions

1f10340

Fix code warnings

50ebfc6

hu55a1n1 approved these changes Apr 22, 2022

View reviewed changes

plafer approved these changes Apr 22, 2022

View reviewed changes

hu55a1n1 merged commit 5e886ca into informalsystems:master Apr 22, 2022

Wizdave97 deleted the david/ics-20-keeper-proposal branch April 22, 2022 16:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ICS20 ADR Proposal #1884

ICS20 ADR Proposal #1884

Wizdave97 commented Feb 17, 2022 •

edited by hu55a1n1

Loading

hu55a1n1 commented Feb 17, 2022

hu55a1n1 left a comment

hu55a1n1 Feb 18, 2022

hu55a1n1 Feb 18, 2022

hu55a1n1 Feb 18, 2022

hu55a1n1 Feb 18, 2022

hu55a1n1 Feb 18, 2022

hu55a1n1 Feb 18, 2022

hu55a1n1 Feb 18, 2022

hu55a1n1 Feb 18, 2022

hu55a1n1 Feb 18, 2022

hu55a1n1 Feb 18, 2022

hu55a1n1 Feb 18, 2022

Wizdave97 commented Feb 18, 2022

hu55a1n1 Feb 21, 2022 •

edited

Loading

hu55a1n1 Feb 21, 2022 •

edited

Loading

Wizdave97 commented Apr 5, 2022

adizere commented Apr 5, 2022

Wizdave97 commented Apr 7, 2022

plafer Apr 13, 2022

plafer Apr 13, 2022

adizere Apr 20, 2022

plafer Apr 13, 2022

plafer Apr 13, 2022

plafer Apr 13, 2022

plafer Apr 13, 2022

plafer Apr 13, 2022

adizere left a comment

adizere Apr 20, 2022

hu55a1n1 left a comment

plafer left a comment


		The following handlers are recommended to be implemented in the ics20_fungible_token_transfer application in the ibc-rs crate

		These handlers will be executed in the module callbacks of any thirdparty IBCmodule that is implementing an ICS20 application on-chain

ICS20 ADR Proposal #1884

ICS20 ADR Proposal #1884

Conversation

Wizdave97 commented Feb 17, 2022 • edited by hu55a1n1 Loading

Description

PR author checklist:

Reviewer checklist:

hu55a1n1 commented Feb 17, 2022

hu55a1n1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wizdave97 commented Feb 18, 2022

hu55a1n1 Feb 21, 2022 • edited Loading

Choose a reason for hiding this comment

hu55a1n1 Feb 21, 2022 • edited Loading

Choose a reason for hiding this comment

Wizdave97 commented Apr 5, 2022

adizere commented Apr 5, 2022

Wizdave97 commented Apr 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adizere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hu55a1n1 left a comment

Choose a reason for hiding this comment

plafer left a comment

Choose a reason for hiding this comment

Wizdave97 commented Feb 17, 2022 •

edited by hu55a1n1

Loading

hu55a1n1 Feb 21, 2022 •

edited

Loading

hu55a1n1 Feb 21, 2022 •

edited

Loading