docs: fleshing out ics21 spec #1
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| - Preservation of account permissions crosschain, which can forbid an account from | ||
| 1. Sending the permissioned token | ||
| 2. Receiving the permissioned token | ||
| 3. Using the permmissioned token to pay for gas // todo ? is this needed? |
There was a problem hiding this comment.
Is this needed as a desired property?
| 1. Sending the permissioned token | ||
| 2. Receiving the permissioned token | ||
| 3. Using the permmissioned token to pay for gas // todo ? is this needed? | ||
| 4. Transferring the token back to Host Chain // todo ? should this be disallowed? Should even a blacklisted user be always able to send tokens to themselves on noble? and funds locked there |
There was a problem hiding this comment.
should this be disallowed? Should even a blacklisted user be always able to send tokens to themselves on noble? and funds locked there
There was a problem hiding this comment.
Good call out. IMO, a blacklisted user should be able to send tokens back to their wallet on source, in which, it will then be blacklisted.
There was a problem hiding this comment.
removed this requirement in 4cbea5e
| The Host Chain is responsible for hosting a native token issuance mechanism. Any issued token denom should expose an Owner which controls the token issuance. Any number of ICS20 channels can be created between Host Chain and Mirror Chain for the transfer of these tokens. When the Owner decides to make the token permissioned, they register it with the ICS21 Host Module. | ||
|
|
||
| Once a Permissioned Token has been registered on the Host Chain, the Owner can now set the persmissions customization. This would include: | ||
| 1. The ICS20 Channel IDs over which the Permissioned Token denom can be sent or received. // todo: is there any need to do receive check? should send check be enough? this will solve the potential issue in the 2nd paragraph below |
There was a problem hiding this comment.
is there any need to do receive check? should send check be enough? this will solve the potential issue in the 2nd paragraph below
| counterpartyChannelIdentifier: Identifier, | ||
| version: string | ||
| ): (version: string, err: Error) { | ||
| // only unordered channels allowed. this is bcuz ordered channels close on timeout |
There was a problem hiding this comment.
only unordered channels allowed. this is bcuz ordered channels close on timeout we dont want that. do we? but also, because we only submit the changeset of permissions and not the entire permission set, Ordered channels would make sense, to ensure n packet is accepted before n+1. e.g we add Alice to bloccklist in n, but remove Alice in n+1. if they are processed in wrong order, we will end up with state where Alice is still blocked
| } | ||
| ``` | ||
|
|
||
| #### Channel Closing Flow |
There was a problem hiding this comment.
who can initiate channel close? just host? should mirror be able to exit as well (probably not). why would hosts want to close? should we have conditions that all AllowedChannels store should be empty (wrt channels on that chain) to be able to close. If not, Should we only allow the close of the channel if there are no ics20tokens of denom across the channel? Is this entrypoint hit when an ordered channels is closed due to timeout? will need to address channel closure due to client expiry anyway
| if data.Signer != authtypes.NewModuleAddress(ics21types.ModuleName+data.CounterpartyChannelId) { | ||
| return NewErrorAcknowledgement(ics21types.ErrInvalidSigner) | ||
| } | ||
| /// todo: curent implementation also shares the whitelisted channel IDs to mirror. do we need to store this in Mirror? if we know a token is permissioned, cant we just ensure that it cant do ibc trasnfer to any chain except the source chain? i.e it can only return on the channel it came from, nothing else |
There was a problem hiding this comment.
current implementation also shares the whitelisted channel IDs to mirror. do we need to store this in Mirror? if we know a token is permissioned, cant we just ensure that it cant do ibc trasnfer to any chain except the source chain? i.e it can only return on the channel it came from, nothing else
| } | ||
|
|
||
| switch typeof(acknowledgement) { | ||
| case *channeltypes.Acknowledgement_Error: |
There was a problem hiding this comment.
this is how the current implementation handles this, but in case of single Host but multiple Mirrors, this would mean, in case one channel failed to update but all others succeeded, the Host would still reset the state and end up with state mismatch.
|
|
||
| ```typescript | ||
| // Called on Host Chain by Relayer | ||
| function onTimeoutPacket(packet: Packet) { |
There was a problem hiding this comment.
same as above. in case of Ordered channels this would close the channel. should handle that. but in general, how to handle? re-attempt to send permissions update again?
There was a problem hiding this comment.
A timed out packet would close the ics21 channel if channels are ordered?
There was a problem hiding this comment.
yea its done to ensure the integrity of the sequence number. e.g if packet n times out, counterparty never learns about it. next packet will have n+1 seq number, and counterparty cant trust this packet cuz it doesnt know what happened to n packet.
| permissions.ChannelAllowlist.pop(channel) | ||
| } | ||
|
|
||
| SetPermissions(denom, permissions) |
There was a problem hiding this comment.
should we consider initiating a new ICS21 channel here itself for every allowedChannel? this would mean for every Permissioned Token, there will be a dedicated channel. We could store a mapping of denom -> ics21 channels across all chains. Easy lookup for when permissions need to be updated everywhere. This would also allow to use Ordered Channels without ending up in a situation where the channel closes for all due to one timeout. so it reduces surface area of that kinda issues. we could trigger channel closes for removedChannels too
| 1. Sending the permissioned token | ||
| 2. Receiving the permissioned token | ||
| 3. Using the permmissioned token to pay for gas // todo ? is this needed? | ||
| 4. Transferring the token back to Host Chain // todo ? should this be disallowed? Should even a blacklisted user be always able to send tokens to themselves on noble? and funds locked there |
There was a problem hiding this comment.
Good call out. IMO, a blacklisted user should be able to send tokens back to their wallet on source, in which, it will then be blacklisted.
| 1. Host Chain to Mirror Chain using a channel in ChanneAllowlist | ||
| 2. Mirror Chain to Host Chain across the channel it came from | ||
|
|
||
| - Application of the permissions to tokens which have already been transferred before the configuration of ICS21 e.g multiple ics20 channels created and amount transferred before the creation of ICS21 channel. these should be addressed or at least identified |
There was a problem hiding this comment.
IMO, this is at the fault of the ICS21 implementor.
ICS21 should not be accountable for IBC transactions prior to the chain implementing the ics21 module(s).
| accountblocklist_additions: bytes[] | ||
| // accountblocklist_removals is the list of pubkeys removed from the blocklist with | ||
| // the new permissions update by the Owner | ||
| accountblocklist_removals: string[] |
| // Ensure the denom is registered with the x.bank module | ||
| denomExists = GetDenomFromBank(denom) | ||
| abortTransactionUnless(denomExists == true) |
There was a problem hiding this comment.
I think a chain should be allowed to permission a token that they haven't yet registered. I also think a chain should be able to permission a non native token.
However I can be swayed in either direction.
There was a problem hiding this comment.
If permissions can be set for a token not yet registered with bank, could it allow Squatting? like if i know a new denom is launching on noble, I could just set myself as owner here right now 🤔
regarding setting non-native tokens to be permissioned, i am wondering if there could be potential issues if Host1 creates a permissioned token, sends it to Mirror1, and then mirror1 decides to set it as permissioned on itself as Host2. and what kinda issues this could create.
| // allowedChannels is a list of ICS20 channels IDs over which the denom can be sent over | ||
| allowedChannels: string[], | ||
| // removedChannels is a list of ICS20 channel IDs over which the denom cannot be sent over anymore | ||
| removedChannels: string[] |
There was a problem hiding this comment.
To reduce complexity, especially when updating the counterparty chain, maybe we separate the adding and removing of channels into separate messages/ibc packets?
This is at least how I currently have it implemented.
I also do not have this as a list of channels, and only allow the user to add or remove one channel at a time.
Do we envision users permssioning/forbidding multiple channels at once on one chain?
This would require a lot of checks and a lot of walking back state if say the update failed on the mirror chain.
But again, I can be convinced if we deem all this functionality should be combined.
There was a problem hiding this comment.
I think the ChannelAllowlist needs to only exist on Host Chain. Because we arent not allowing multihops right. So the Mirror chain will forbid the token from going across any channel which is not the Host channel that it came from.
so not sure if this needs to be shared in an IBC packet
|
|
||
| ```typescript | ||
| // Called on Host Chain by Relayer | ||
| function onTimeoutPacket(packet: Packet) { |
There was a problem hiding this comment.
A timed out packet would close the ics21 channel if channels are ordered?
|
Hey @boojamya
If we wanna use option 1, things can stay as they are. But I am pro option 2, cuz it involves lesser data transfer. (in case over time the permissions object could get very big that the relayer fees get too high or might even exceed max_tx_bytes ) ICS21 token flow
for _, ics20channel in msg.allowedChannels {
// check if its a valid ics20 channel with transfer port
connectionID = connectionkeeper.GetConnectionByChannel(ics20channel)
// check in local x/host state on if there is an ics21 channel for this denom on this connection already
if exists, continue
else, {
// we force an ics21 channel creation immediately as we know we need it anyway
port = ics21host-<denom>
connectionid = ics20connectionID
capability = portkeeper.Bindport(port)
claimcapability(port, capability)
msg := channeltypes.NewMsgChannelOpenInit(port, "ics21-1", ORDERED, []string{connectionID}, "ics21mirror", owner)
handler := k.msgRouter.Handler(msg)
res, err := handler(ctx, msg)
events := res.GetEvents()
ctx.EventManager().EmitEvents(events) // need this so relayers can pick it up
}
}
updatePacket = ics21types.SetAccountBlocklistPacket {
denom = denom,
accountblocklist_additions = addToBlocklist
accountblocklist_removals = removeFromBlocklist
}
allics21channels = hostkeeper.Get(denom)
for _,ics21channel in allICS21channels {
handler.SendPacketOnChannel(channel, updatePacket)
}Channel handshakeon chanopeninit -> parse denom from portid. check that denom is registered to be permissioned. ensure the signer is the token owner and not a random relayer. - this makes it permissioned. (but thats fine i think? because this channel is just for this denom) Relayon RecvPacket - mirrorKeeper.Set(ics21channelid, permissions) Mirror contractWith these two contracts, we can apply the ics21 permissions within the SendRestrictions as well as the ante handler func IsPermissionedToken(denom) bool {
permissions := GetTokenPermissions(denom)
return permissoins != nil
}
func GetTokenPermissions(denom) {
// denom will be ibc path as this is mirror
basedenom, ics20ChannelID := transfertypes.ParseDenomTrace(denom)
connectionID := ibcconnectionkeeper.GetConnection(ics20channelID)
ics21channel := mirrorKeeper.Get(baseDenom, connectionID)
permissions := mirrorKeeper.Get(ics21channel)
return permissions
} |
|
@spoo-bar, I really like the idea of initiating the I've begun implementing here https://github.com/noble-assets/ics21/blob/733c23c6af2a782101b6678e362854ecada567c5/x/host/keeper/msg_server.go#L46-L106 While I've made some changes to state and the IBC channel handshake, the core idea of one channel per denom and initiating the channel handshake inside the Looking forward to discussing this more when you get back! |
Render: https://github.com/noble-assets/ibc/tree/spoorthi/ics21-update/spec/app/ics-021-permissioned-token-transfer
Note: There are likely many typos. pls ignore for now 🙏🏻