-
Notifications
You must be signed in to change notification settings - Fork 1
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
add UIP-5: Outbound Packet Forwarding Middleware Support #4
Open
avahowell
wants to merge
2
commits into
main
Choose a base branch
from
pfm-uip
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,118 @@ | ||
| uip | 5 | | ||
| - | - | | ||
| title | Outbound Packet Forwarding Middleware Support | | ||
| description | Add configurable memo field to Ics20Withdrawal to support outbound multi-hop ICS20 transfers using Packet Forwarding Middleware | | ||
| author | Ava Howell | | ||
| discussions-to | [URL] | | ||
| status | Draft | | ||
| type | Standards Track | | ||
| consensus | No | | ||
| created | 2024-11-06 | | ||
|
||
## Abstract | ||
|
||
This specification adds support for outbound transfers from Penumbra that are routed across multiple interchain hops with the interchain standard IBC Packet Forwarding Middleware (PFM). By adding a user-configurable memo field to the Ics20Withdrawal transaction type, an outbound transaction can specify a multi-chain withdrawal path, greatly simplifying the UX for withdrawals that require a multi-hop interchain path. | ||
|
||
## Motivation | ||
|
||
The IBC Packet Forwarding Middleware (PFM) enables seamless multi-hop token transfers across IBC-connected chains. This is accomplished through the usage of the memo field in the `FungibleTokenPacketData` struct that is used to encode the details of a transfer. When a multi-hop withdrawal is initiated, the user specifies the details of the route that the packet should take by JSON-encoding a struct inside the FungibleTokenPacketData's memo field. Currently, Penumbra hardcodes an empty string for the memo field in ICS20 transfers, preventing users from specifying the routing information required for PFM. The minimal protocol-level change to support outbound multi-hop packets from penumbra is to add memo support to Ics20Withdrawal actions, allowing users to take advantage of more complex IBC routing paths while maintaining their privacy. | ||
|
||
## Specification | ||
|
||
The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED", "MAY", and "OPTIONAL" in this document are to be interpreted as described in RFC 2119 and RFC 8174. | ||
|
||
### Ics20Withdrawal Changes | ||
|
||
The `Ics20Withdrawal` struct and associated protobuf types shall be modified to add a new `ics20_memo` field: | ||
|
||
```rust | ||
pub struct Ics20Withdrawal { | ||
// Existing fields... | ||
pub amount: Amount, | ||
pub denom: asset::Metadata, | ||
pub destination_chain_address: String, | ||
pub return_address: Address, | ||
pub timeout_height: IbcHeight, | ||
pub timeout_time: u64, | ||
pub source_channel: ChannelId, | ||
pub use_compat_address: bool, | ||
|
||
// New field | ||
pub ics20_memo: String, | ||
} | ||
``` | ||
|
||
The `From<Ics20Withdrawal> for pb::FungibleTokenPacketData` impl, which encodes the withdrawal in the ICS-compatible packet data type, SHALL be modified to include the contents of this new field: | ||
|
||
```rust | ||
impl From<Ics20Withdrawal> for pb::FungibleTokenPacketData { | ||
fn from(w: Ics20Withdrawal) -> Self { | ||
let return_address = match w.use_compat_address { | ||
true => w.return_address.compat_encoding(), | ||
false => w.return_address.to_string(), | ||
}; | ||
|
||
pb::FungibleTokenPacketData { | ||
amount: w.value().amount.to_string(), | ||
denom: w.denom.to_string(), | ||
receiver: w.destination_chain_address, | ||
sender: return_address, | ||
memo: w.ics20_memo, | ||
} | ||
} | ||
} | ||
``` | ||
|
||
The `ics20_memo` field: | ||
- MAY contain any valid UTF-8 string | ||
- MAY be empty | ||
|
||
## Rationale | ||
|
||
There are two major parts of Packet Forwarding Middleware support: | ||
|
||
- Outbound PFM Support (what this UIP supports): the support for sending packets that can be autonomously routed over multiple intermediate chains before reaching their destination. | ||
- Packet Forwarding Middleware support: the actual forwarding middleware implementation, which runs in the chain's state machine and allows the chain to forward packets sent from other chains. | ||
|
||
This UIP implements (1), using a minimal protocol change (adding the configurable memo field). This gives the majority of end-user benefit from PFM support, notably that users can experience superior UX in withdrawing from the Penumbra chain, while avoiding complex protocol development work and assurance. A separate UIP for implementing (2) in the Penumbra IBC stack could be considered in the future. | ||
|
||
|
||
## Backwards Compatibility | ||
|
||
This UIP adds an additional field to the Ics20Withdrawal struct, which would be a breaking change and require a protocol upgrade. | ||
|
||
## Test Cases | ||
|
||
```rust | ||
#[test] | ||
fn test_ics20_withdrawal_memo() { | ||
let withdrawal = Ics20Withdrawal { | ||
amount: Amount::from(1000u64), | ||
denom: asset::Metadata::dummy(), | ||
destination_chain_address: "cosmos1...".to_string(), | ||
return_address: Address::dummy(), | ||
timeout_height: IbcHeight::new(1, 1000), | ||
timeout_time: 1000000, | ||
source_channel: ChannelId::new(0), | ||
use_compat_address: false, | ||
ics20_memo: "{\"forward\":{...}".to_string(), | ||
}; | ||
|
||
let packet_data: FungibleTokenPacketData = withdrawal.into(); | ||
assert_eq!(packet_data.memo, withdrawal.ics20_memo); | ||
} | ||
``` | ||
|
||
## Security Considerations | ||
|
||
This change does not introduce new security considerations at the IBC state machine level. The memo field is already part of the ICS20 specification, this UIP simply opens up the possibility for the user to configure its contents while building a withdrawal transaction. | ||
|
||
Clients implementing PFM support should be aware of the following considerations: | ||
|
||
1. Address compatibility requirements for PFM refunds are handled at the client layer. | ||
2. Timeout and error handling handling in multi-hop transfers may require specific client-side logic to ensure address compatibility. | ||
3. The memo contents should be validated by clients to ensure they match PFM specifications. | ||
|
||
## Copyright | ||
|
||
Copyright and related rights waived via [CC0](https://github.com/penumbra-zone/UIPs/blob/main/LICENSE). |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Doesn't this open up a security consideration for Penumbra? Currently, we don't have any transaction fields that allow user- (attacker-) controlled lengths; this would be one.
The ICS20 memo field has been used for griefing on other chains. I think this won't be a problem, because Penumbra charges transactions gas for bytes used in the transaction encoding, so an attacker would have to pay to spam the chain, and the gas cost can change / be changed to respond to an attacker. But we should note that in the UIP, I think, so there's a documented rationale.
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 we consider adding a maximum size bound to the memo field in stateless checks for Ics20Withdrawal? This isn't specified in the ICS spec as far as I am aware, but a reasonable upper bound could be added with likely few interoperability concerns.
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'm in favor of not adding a bound a priori, and just relying on gas ; god knows what legitimate use cases people might come up with. For example, a priori many people assumed that 90 bech32 characters were sufficient for any kind of address.