Skip to content
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

ICS20-2: Add Token Forwarding ability to ICS20-2 #1090

Merged
merged 28 commits into from
Jun 24, 2024
Merged
Changes from 3 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
a40890b
add recv logic and revert logic on error ack/timeout
AdityaSripal Mar 26, 2024
b10ba74
multiple denoms but still support forwarding
AdityaSripal Mar 28, 2024
1de6ca0
lint
crodriguezvega Apr 2, 2024
675ccd8
Apply suggestions from code review
AdityaSripal Apr 8, 2024
6b565d0
address reviews
AdityaSripal Apr 8, 2024
78e06a4
fix merge
AdityaSripal Apr 8, 2024
49f4b5d
Apply suggestions from code review
AdityaSripal Apr 8, 2024
b702942
small fixes
AdityaSripal Apr 8, 2024
3fcb972
generate forwarding address if it doesn't exist
AdityaSripal Apr 8, 2024
a278f1b
blank memo for now
AdityaSripal Apr 11, 2024
4e9547e
only do revertInFlightChanges or refund packets on error ack, not both
AdityaSripal Apr 18, 2024
a42ea6e
make same changes for timeout
AdityaSripal Apr 18, 2024
2d98d83
forwarding info struct, individual memos
AdityaSripal Apr 22, 2024
eb6fbf5
fail early since forwarding must be atomic
AdityaSripal Apr 24, 2024
ab34a49
address some small review comments
crodriguezvega May 2, 2024
055ae83
only set memo on last hop
AdityaSripal May 7, 2024
2e0f5da
fix merge
AdityaSripal May 7, 2024
a759ca6
address Stefano's and my review comments
crodriguezvega May 30, 2024
2a71fbd
missing bracket
crodriguezvega Jun 5, 2024
0b4f63f
destinationChannel -> destChannel
crodriguezvega Jun 11, 2024
eb6bd36
ICS20v2 path forwarding: simplify revert in-flight changes (#1110)
crodriguezvega Jun 19, 2024
ec5fcc3
nit: rename ForwardingInfo to Forwarding (#1112)
crodriguezvega Jun 19, 2024
89aec20
ICS20v2: use protobuf encoding for packet data of v2 (#1118)
crodriguezvega Jun 19, 2024
58b3462
delete previously forwarded packet (#1119)
crodriguezvega Jun 19, 2024
77414dc
Update spec/app/ics-020-fungible-token-transfer/README.md
AdityaSripal Jun 20, 2024
712721b
Apply suggestions from code review
AdityaSripal Jun 20, 2024
c667418
ICS20v2: use `Denom` and `Trace` types (#1115)
crodriguezvega Jun 24, 2024
e613e77
review comment
crodriguezvega Jun 24, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
166 changes: 156 additions & 10 deletions spec/app/ics-020-fungible-token-transfer/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ interface FungibleTokenPacketDataV2 {
sender: string
receiver: string
memo: string
forwardingPath: []string
crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved
}

interface Token {
Expand Down Expand Up @@ -92,6 +93,7 @@ The fungible token transfer bridge module tracks escrow addresses associated wit
```typescript
interface ModuleState {
channelEscrowAddresses: Map<Identifier, string>
channelForwardingAddresses: Map<Identifier, string>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this actually needed? It seems like we could manage by using channelEscrowAddresses only?

Copy link
Contributor

@sangier sangier Apr 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I am missing something about the reasoning behind the separation

channelEscrowAddresses: Map<Identifier, string>
channelForwardingAddresses: Map<Identifier, string>

I agree we could use directly channelEscrowAddresses. The channelEscrowAddresses mapping is populated during onChanOpenInit at line 157 or onChainOpenTry at line 181. That means we could potentially access this mapping at line 331 using the packet.destintionChannel as identifier.
Instead the channelForwardingAddresses is never set. Thus the read we do at line 331 may be illegal.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the only reason for using this is a logical separation, I think I would be in favour of simply using the escrow address. Having this other set of addresses adds to the overall complexity without adding a huge amount of value IMO.

And like @damiannolan is linking, the existing escrow address has already been reasoned about in terms of collisions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The forwarding address is mainly just used as pass through storage based on how ics20 is constructed in the non-forwarding case. I think the spec just needs to specify that this address should be an address the transfer module controls. I don't think it should reuse the escrow addresses

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I am missing something, I can't think of a reason why you need channel isolated forwarding addresses cosmos/ibc-go#6561 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I decided to split it off to a separate address is just an additional security blanket. The balance of the escrow addresses is security critical. If the balance ends up in an invalid state, that can be a very painful issue to unwind.

It should be completely fine if there's no bugs. But if there's a bug in forwarding, i figure having a separate account might make the impact a bit more isolated.

Of course it is receiving from and sending back to the escrow addresses, so if there's issues there it could still be a problem for escrow account balances.

I'm ok either way. I just figured to start out with a separate account for clarity and avoiding having the escrow account representing more than one thing semantically.

We can use the same account as well as @colin-axner suggests

}
```

Expand Down Expand Up @@ -241,6 +243,7 @@ function sendFungibleTokens(
sender: string,
receiver: string,
memo: string,
forwardingPath: []string,
sourcePort: string,
sourceChannel: string,
timeoutHeight: Height,
Expand Down Expand Up @@ -269,11 +272,16 @@ function sendFungibleTokens(
transferVersion = getAppVersion(channel.version)
if transferVersion == "ics20-1" {
abortTransactionUnless(len(tokens) == 1)
v1Denom = tokens[0].trace + tokens[0].denom)
data = FungibleTokenPacketData{v1Denom, tokens[0].amount, sender, receiver, memo}
token = tokens[0]
// abort if forwardingPath defined
abortTransactionUnless(forwardingPath == nil)
// create v1 denom of the form: port1/channel1/port2/channel2/port3/channel3/denom
v1Denom = constructOnChainDenom(token.trace, token.denom)
// v1 packet data does not support forwardingPath fields
data = FungibleTokenPacketData{v1Denom, token.amount, sender, receiver, memo}
} else if transferVersion == "ics20-2" {
// create FungibleTokenPacket data
data = FungibleTokenPacketDataV2{tokens, sender, receiver, memo}
data = FungibleTokenPacketDataV2{tokens, sender, receiver, memo, forwardingPath}
} else {
// should never be reached as transfer version must be negotiated to be either
// ics20-1 or ics20-2 during channel handshake
Expand Down Expand Up @@ -302,6 +310,8 @@ function onRecvPacket(packet: Packet) {
// getAppVersion returns the transfer version that is embedded in the channel version
// as the channel version may contain additional app or middleware version(s)
transferVersion = getAppVersion(channel.version)
var tokens []Token
var receiver string
if transferVersion == "ics20-1" {
FungibleTokenPacketData data = UnmarshalJSON(packet.data)
trace, denom = parseICS20V1Denom(data.denom)
crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -311,26 +321,36 @@ function onRecvPacket(packet: Packet) {
amount: packet.amount
crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved
}
tokens = []Token{token}
receiver = packet.receiver
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved
} else if transferVersion == "ics20-2" {
FungibleTokenPacketDataV2 data = UnmarshalJSON(packet.data)
tokens = data.tokens
// if we need to forward the tokens onward
// temporarily send to the channel escrow address of the intended receiver
if len(forwardingPath) > 0 {
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved
receiver = channelForwardingAddresses[packet.destinationChannel]
crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved
}
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved
} else {
// should never be reached as transfer version must be negotiated to be either
// ics20-1 or ics20-2 during channel handshake
abortTransactionUnless(false)
}

assert(packet.sender !== "")
crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved
assert(receiver !== "")
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved

// construct default acknowledgement of success
FungibleTokenPacketAcknowledgement ack = FungibleTokenPacketAcknowledgement{true, null}

prefix = "{packet.sourcePort}/{packet.sourceChannel}/"
receivedTokens = []Token
for token in tokens {
assert(token.denom !== "")
assert(token.amount > 0)
assert(token.sender !== "")
assert(token.receiver !== "")


// we are the source if the packets were prefixed by the sending chain
source = token.trace[0] == prefix
var onChainTrace []string
if source {
// since we are receiving back to source we remove the prefix from the trace
onChainTrace = token.trace[1:]
Expand All @@ -339,7 +359,7 @@ function onRecvPacket(packet: Packet) {
// determine escrow account
escrowAccount = channelEscrowAddresses[packet.destChannel]
// unescrow tokens to receiver (assumed to fail if balance insufficient)
err = bank.TransferCoins(escrowAccount, data.receiver, onChainDenom, token.amount)
err = bank.TransferCoins(escrowAccount, receiver, onChainDenom, token.amount)
if (err != nil) {
ack = FungibleTokenPacketAcknowledgement{false, "transfer coins failed"}
// break out of for loop on first error
Expand All @@ -348,17 +368,55 @@ function onRecvPacket(packet: Packet) {
} else {
// since we are receiving to a new sink zone we prepend the prefix to the trace
prefix = "{packet.destPort}/{packet.destChannel}/"
newTrace = append([]string{prefix}, token.trace...)
onChainDenom = constructOnChainDenom(newTrace, token.denom)
onChainTrace = append([]string{prefix}, token.trace...)
onChainDenom = constructOnChainDenom(onChainTrace, token.denom)
// sender was source, mint vouchers to receiver (assumed to fail if balance insufficient)
err = bank.MintCoins(data.receiver, onChainDenom, token.amount)
err = bank.MintCoins(receiver, onChainDenom, token.amount)
if (err !== nil) {
ack = FungibleTokenPacketAcknowledgement{false, "mint coins failed"}
// break out of for loop on first error
break
}
}

// add the received token to the received tokens list
recvToken = Token{
denom: token.denom,
trace: onChainTrace,
amount: token.amount,
}
receivedTokens = append(receivedTokens, recvToken)
}

// if there is an error ack return immediately and do not forward further
if !ack.Success() {
return ack
}

// if acknowledgement is successful and forwarding path set
// then start forwarding
if forwardingPath != nil {
crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved
nextPort, nextChannel = split(forwardingPath[0], "/")
// send the tokens we received above to the next port and channel
// on the forwarding path
// and reduce the forwardingPath by the first element
nextSequence = sendFungibleTokens(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
nextSequence = sendFungibleTokens(
nextPacketSequence = sendFungibleTokens(

crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved
tokens: receivedTokens,
sangier marked this conversation as resolved.
Show resolved Hide resolved
sender: receiver,
sangier marked this conversation as resolved.
Show resolved Hide resolved
receiver: data.receiver,
crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved
sangier marked this conversation as resolved.
Show resolved Hide resolved
memo,
forwardingPath[1:],
nextPort,
nextChannel,
Height{},
currentTime() + DefaultHopTimeoutPeriod,
)
// store packet for future sending ack
store.set(packetForwardPath(nextPort, nextChannel, nextPacketSequence), packet)
sangier marked this conversation as resolved.
Show resolved Hide resolved
// use async ack until we get successful acknowledgement from further down the line.
return nil
}

return ack
}
```
Expand All @@ -373,6 +431,30 @@ function onAcknowledgePacket(
if !(acknowledgement.success) {
refundTokens(packet)
}

// check if the packet was sent is from a previously forwarded packet
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved
prevPacket = store.get(packetForwardPath(packet.sourcePort, packet.sourceChannel))
sangier marked this conversation as resolved.
Show resolved Hide resolved

if prevPacket != nil {
if acknowledgement.success {
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved
FungibleTokenPacketAcknowledgement ack = FungibleTokenPacketAcknowledgement{true, "forwarded packet succeeded"}
handler.writeAcknowledgement(
prevPacket,
ack,
)
} else {
// the forwarded packet has failed, thus the funds have been refunded to the forwarding address.
// we must revert the changes that came from successfully receiving the tokens on our chain
// before propogating the error acknowledgement back to original sender chain
revertInFlightChanges(packet, prevPacket)
// write error acknowledgement
FungibleTokenPacketAcknowledgement ack = FungibleTokenPacketAcknowledgement{false, "forwarded packet failed"}
handler.writeAcknowledgement(
prevPacket,
ack,
)
sangier marked this conversation as resolved.
Show resolved Hide resolved
}

}
```

Expand All @@ -382,6 +464,31 @@ function onAcknowledgePacket(
function onTimeoutPacket(packet: Packet) {
// the packet timed-out, so refund the tokens
refundTokens(packet)
crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved

// check if the packet was sent is from a previously forwarded packet
prevPacket = store.get(packetForwardPath(packet.sourcePort, packet.sourceChannel))

if prevPacket != nil {
// the forwarded packet has failed, thus the funds have been refunded to the forwarding address.
// we must revert the changes that came from successfully receiving the tokens on our chain
// before propogating the error acknowledgement back to original sender chain
revertInFlightChanges(packet, prevPacket)
// write error acknowledgement
FungibleTokenPacketAcknowledgement ack = FungibleTokenPacketAcknowledgement{false, "forwarded packet failed"}
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved
handler.writeAcknowledgement(
prevPacket,
ack,
)
}
}
```

##### Helper functions

```typescript
function isSource(portId: string, channelId: string, token: Token) {
firstTrace = "{portId}/{channelId}"
return token.trace[0] == firstTrace
}
```

Expand Down Expand Up @@ -428,6 +535,45 @@ function refundTokens(packet: Packet) {
}
```

```typescript
// revertInFlightChanges reverts the receive packet and send packet
// that occurs in the middle chains during a packet forwarding
// If an error occurs further down the line, the state changes
// on this chain must be reverted before sending back the error acknowledgement
// to ensure atomic packet forwarding
function revertInFlightChanges(sentPacket: Packet, receivedPacket: Packet) {
// the token on our chain is the token in the sentPacket
for token in sentPacket.tokens {
// check if the packet we sent out was sending as source or not
// in this case we escrowed the outgoing tokens
if isSource(sentPacket.sourcePort, sentPacket.sourceChannel, token) {
// check if the packet we received was a source token for our chain
if isSource(receivedPacket.destinationPort, receivedPacket.desinationChannel, token) {
// receive sent tokens from the received escrow to the forward escrow account
sangier marked this conversation as resolved.
Show resolved Hide resolved
// so we must send the tokens back from the forward escrow to the original received escrow account
sangier marked this conversation as resolved.
Show resolved Hide resolved
forwardEscrow = channelEscrowAddresses[sentPacket.sourceChannel]
reverseEscrow = channelEscrowAddresses[packet.destChannel]
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved
bank.TransferCoins(forwardEscrow, reverseEscrow, token.denom, token.amount)
} else {
// receive minted vouchers and sent to the forward escrow account
sangier marked this conversation as resolved.
Show resolved Hide resolved
// so we must remove the vouchers from the forward escrow account and burn them
sangier marked this conversation as resolved.
Show resolved Hide resolved
bank.BurnCoins(forwardEscrow, token.denom, token.amount)
sangier marked this conversation as resolved.
Show resolved Hide resolved
}
} else {
// in this case we burned the vouchers of the outgoing packets
// check if the packet we received was a source token for our chain
// in this case, the tokens were unescrowed from the reverse escrow account
if isSource(receivedPacket.destinationPort, receivedPacket.desinationChannel, token) {
// in this case we must mint the burned vouchers and send them back to the escrow account
sangier marked this conversation as resolved.
Show resolved Hide resolved
bank.MintCoins(reverseEscrow, token.denom, token.amount)
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved
}
// if it wasn't a source token on receive, then we simply had minted vouchers and burned them in the receive.
sangier marked this conversation as resolved.
Show resolved Hide resolved
// So no state changes were made, and thus no reversion is necessary
sangier marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
```

```typescript
function onTimeoutPacketClose(packet: Packet) {
// can't happen, only unordered channels allowed
Expand Down
Loading