Support setting a shielded address as refund target #4233

yito88 · 2025-01-13T16:12:10Z

Describe your changes

Closes #4168

Based on #4134

Diffs: https://github.com/anoma/namada/compare/bat+tiago/feat/ibc-pfm-with-invalid-addrs..yuji/ibc-shielded-refund

Checklist before merging

If this PR has some consensus breaking changes, I added the corresponding breaking:: labels
- This will require 2 reviewers to approve the changes
If this PR requires changes to the docs or specs, a corresponding PR is opened in the namada-docs repo
- Relevant PR if applies:
If this PR affects services such as namada-indexer or namada-masp-indexer, a corresponding PR is opened in that repo
- Relevant PR if applies:

Co-authored-by: Jacob Turner <[email protected]>

Co-authored-by: Jacob Turner <[email protected]> Co-authored-by: Yuji Ito <[email protected]>

Co-authored-by: Jacob Turner <[email protected]>

codecov · 2025-01-13T16:50:25Z

Codecov Report

Attention: Patch coverage is 44.18605% with 600 lines in your changes missing coverage. Please review.

Project coverage is 74.40%. Comparing base (c0b9f68) to head (9ced974).

Files with missing lines	Patch %	Lines
crates/ibc/src/lib.rs	30.71%	185 Missing ⚠️
crates/ibc/src/context/middlewares/pfm_mod.rs	52.63%	180 Missing ⚠️
crates/sdk/src/tx.rs	0.00%	79 Missing ⚠️
crates/sdk/src/masp.rs	22.58%	48 Missing ⚠️
crates/ibc/src/context/nft_transfer_mod.rs	5.26%	36 Missing ⚠️
crates/ibc/src/msg.rs	48.88%	23 Missing ⚠️
crates/ibc/src/context/common.rs	50.00%	14 Missing ⚠️
crates/ibc/src/context/transfer_mod.rs	63.15%	14 Missing ⚠️
crates/sdk/src/rpc.rs	0.00%	12 Missing ⚠️
crates/ibc/src/context/token_transfer.rs	63.63%	8 Missing ⚠️
... and 1 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4233      +/-   ##
==========================================
- Coverage   74.59%   74.40%   -0.19%     
==========================================
  Files         342      344       +2     
  Lines      108724   109587     +863     
==========================================
+ Hits        81101    81543     +442     
- Misses      27623    28044     +421

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

grarco · 2025-01-21T12:08:54Z

crates/tests/src/e2e/ibc_tests.rs

@@ -3218,3 +3288,29 @@ fn packet_forward_memo(
    })
    .expect("Test failed")
 }
+
+fn gen_refund_target(test: &Test, alias: &str) -> Result<()> {


This should really be a disposable address but we don't have the right support for it in the wallet. I'll add a note to #4040

grarco · 2025-01-21T13:58:12Z

crates/core/src/masp.rs

@@ -104,7 +104,7 @@ impl Display for MaspTxId {
    Serialize,
    Deserialize,
 )]
-pub struct MaspEpoch(Epoch);
+pub struct MaspEpoch(pub Epoch);


This has been made public to extract the inner Epoch when constructing the refund storage key. Can't we instead implement KeySeg on MaspEpoch? Otherwise could we come up with a getter to extract the inner epoch from here? I'd like to avoid exposing the inner type as the whole point of this type was to ensure safe operations on it

grarco · 2025-01-21T14:53:42Z

crates/ibc/src/lib.rs

+                MsgEnvelope::Packet(PacketMsg::Ack(msg)) => {
+                    let refund_info = recv_info_from_packet(&msg.packet, true)
+                        .map_err(StorageError::new)?;
+                    accum = apply_refund_msg(


Is it ok to apply the refund message without checking if the ack is successful or not?

grarco · 2025-01-21T16:54:35Z

What happens if the shielded refund transaction fails? In the wasm code of the ibc transaction we don't explicitly handle this case. I tried looking at ICS20 but I couldn't find anything in terms of what the refund operation should guarantee or not

sug0 · 2025-01-20T12:10:19Z

crates/ibc/src/actions.rs

        transfer: None,
+        refund_masp_tx: None,


A refund masp tx should only exist if a transfer does too. This should be reflected in the layout of the MsgTransfer struct. Right now you can represent a MsgTransfer with a refund_masp_tx but no transfer.

sug0 · 2025-01-20T12:12:02Z

crates/core/src/masp.rs

@@ -104,7 +104,7 @@ impl Display for MaspTxId {
    Serialize,
    Deserialize,
 )]
-pub struct MaspEpoch(Epoch);
+pub struct MaspEpoch(pub Epoch);


I'm more in favor of adding an accessor method rather than making this public, to discourage building invalid instances of this type.

sug0 · 2025-01-20T12:24:33Z

crates/ibc/src/lib.rs

+    // Shielded refund only happens if MsgAcknowledgement or MsgTimeout is
+    // successful


this comment is a bit confusing. it should only happen in case a timeout is triggered, or we receive an ack failure

sug0 · 2025-01-20T12:33:15Z

crates/ibc/src/lib.rs

+                if let Some((_, (epoch, refund_masp_tx))) =
+                    msg.transfer.as_ref().zip(msg.refund_masp_tx)
+                {


For instance, make transfer: Option<Either<MaspTransaction, (MaspTransaction, Refund)>>, or Option<(MaspTransaction, Option<Refund>)>

sug0 · 2025-01-20T12:35:44Z

crates/ibc/src/lib.rs

+    fn try_get_refund_masp_tx<Transfer: BorshDeserialize>(
+        storage: &S,


This method (or another one) should delete the refund shielding tx from storage. The refund tx is only needed in storage until an ack/timeout is received.

In shielded sync, the shielding tx could be retrieved from the IBC transfer transaction. It can't apply immediately because we don't know if the transfer hasn't failed at that time.
I tried to retrieve the refund shielding tx from storage in the PR for now. (But I have to fix to delete it when the ack is successful.)
Is it possible to have a kind of pending state of the IBC shielded transfer in shielded sync?

What if extract_masp_tx only returned the MASP tx in case the IBC packet was a timeout or ack failure? This way shielded sync wouldn't attempt to apply notes that could be in a pending state. This doesn't require many changes to the client code, but it might require reworking your current node code a bit...

Oh but I guess that still wouldn't solve the original problem, we can't delete the note from storage since we need to fetch during shielded sync. Perhaps we should give up on storing the shielding tx in storage, and carry it in a memo instead?

sug0 · 2025-01-20T12:49:55Z

crates/ibc/src/lib.rs

-                        let packet_data = serde_json::from_slice::<PacketData>(
-                            &msg.packet.data,
-                        )
+            Some(IbcMessage::Envelope(envelope)) => match *envelope {


Suggested change

Some(IbcMessage::Envelope(envelope)) => match *envelope {

// These events are emitted on the receiver

Some(IbcMessage::Envelope(envelope)) => match *envelope {

sug0 · 2025-01-20T12:52:45Z

crates/ibc/src/lib.rs

+        // If there is a transfer to the IBC account, then deduplicate the
+        // balance increase since we already account for it below


I'm not sure what is being duplicated. Could you clarify, please?

sug0 · 2025-01-20T12:53:24Z

crates/ibc/src/lib.rs

+        let token = convert_to_address(ibc_trace).into_storage_result()?;
+        let delta = ValueSum::from_pair(token, amount);


could be moved inside the if-expr below, since they are only used there

sug0 · 2025-01-20T12:54:14Z

crates/ibc/src/lib.rs

+                                    .refund_masp_tx(
+                                        &msg.packet.port_id_on_a,
+                                        &msg.packet.chan_id_on_a,
+                                        msg.packet.seq_on_a,
+                                        masp_epoch,
+                                    )
+                                    .map_err(|e| Error::Context(Box::new(e)))?


perhaps we could delete the refund tx when we call refund_masp_tx

grarco · 2025-01-21T15:07:40Z

crates/ibc/src/lib.rs

+        packet.seq_on_a,
+        masp_epoch,
+    );
+    if !storage.has_key(&refund_masp_tx_key).ok()? {


One note on this: a masp epoch change will trigger a failure of the shielded refund tx only if any of the involved assets is part of the conversion tree. If none of them is, then the tx will succeed at any epoch (provided that the assets have not entered the conversion tree in the meantime): so maybe we should try the shielded refund anyway before defaulting to the the transparent address (or maybe provide the MasEpoch field as an option and always try when None).

An additional note, the masp Transaction could also carry an expiration in the form of a block height: this could cause a rejection by the MASP vp. We should double check how that is handled when the refund tx is created cause for obvious reasons it cannot match the expiration of the original unshielding tx

Can we check if the assets have not entered the conversion tree in the transaction execution?
In the IBC logic, it just transfers(or mints) tokens from the IBC address to the MASP address. We can fall back the target to the transparent address if we know the shielded refund will fail at that time.

grarco · 2025-01-21T15:46:58Z

crates/ibc/src/msg.rs

@@ -86,6 +98,8 @@ pub struct MsgNftTransfer<Transfer> {
    pub message: IbcMsgNftTransfer,
    /// Shieleded transfer for MASP transaction
    pub transfer: Option<Transfer>,
+    /// MASP transaction for refund
+    pub refund_masp_tx: Option<(MaspEpoch, MaspTransaction)>,


Here is where we probably want to remove the epoch or at least make it optional

grarco · 2025-01-21T15:47:29Z

crates/ibc/src/msg.rs

@@ -36,6 +37,8 @@ pub struct MsgTransfer<Transfer> {
    pub message: IbcMsgTransfer,
    /// Shieleded transfer for MASP transaction
    pub transfer: Option<Transfer>,
+    /// MASP transaction for refund
+    pub refund_masp_tx: Option<(MaspEpoch, MaspTransaction)>,


Same as below

grarco · 2025-01-21T16:41:29Z

crates/sdk/src/tx.rs

+                context,
+                masp_transfer_data,
+                None,
+                args.tx.expiration.to_datetime(),


I believe we shouldn't use the same expiration we use for the transaction also for the refund tx. I don't see why anyone might want to set an expiration on a refunding tx so we could just avoid attaching an expiration at all in this case or we should try to come up with a clever way to estimate the delay we expect on the ibc route (if this is even possible)

grarco · 2025-01-21T16:49:04Z

crates/sdk/src/tx.rs

@@ -2781,8 +2783,11 @@ pub async fn build_ibc_transfer(
    // The refund target should be given or created if the source is shielded.
    // Otherwise, the refund target should be None.
    assert!(
-        (args.source.spending_key().is_some() && refund_target.is_some())
-            || (args.source.address().is_some() && refund_target.is_none())
+        (args.source.spending_key().is_some()


Can we return an error instead if panicking in the sdk?

sug0 and others added 22 commits January 13, 2025 13:13

Add PFM related deps

bbc7046

Co-authored-by: Jacob Turner <[email protected]>

Increase the visibility of some IBC code

6a67ca6

Co-authored-by: Jacob Turner <[email protected]>

Implement PFM module

6241217

Co-authored-by: Jacob Turner <[email protected]>

Hook up PFM middleware to ICS-20 transfer module

90c4e7f

Co-authored-by: Jacob Turner <[email protected]>

Add PFM e2e tests

c25bafa

Co-authored-by: Jacob Turner <[email protected]> Co-authored-by: Yuji Ito <[email protected]>

Enable PFM e2e tests in CI

8599d48

Co-authored-by: Jacob Turner <[email protected]>

Changelog for #4082

bbc1cb1

Remove receiver validation on pfm ibc packets

93dc887

Add function to build transfer middlewares stack

ba8dc8d

Move PFM to middlewares

5093c24

Move module wrapper impl

f4373f5

Handle receiving PFM packets with invalid receivers

bbf6014

Changelog for #4134

d0a242f

WIP: set MaspEpoch

ae8bb13

borrow_mut inner

e607d27

WIP: fix masp vp and add test

0d473cd

shielded sync retrieves refund masp tx

fd6c2d3

packet info

882ff5f

nft shielded refund

a9fec3e

refund when timeout

f5f0db4

clean packet parsing

b493382

changelog

9ced974

yito88 force-pushed the yuji/ibc-shielded-refund branch from bbd65d4 to 9ced974 Compare January 13, 2025 16:18

yito88 added breaking: protocol IBC MASP labels Jan 14, 2025

yito88 mentioned this pull request Jan 15, 2025

Shielde refund in IBC anoma/namada-docs#421

Open

yito88 requested review from sug0 and grarco January 16, 2025 09:31

grarco reviewed Jan 21, 2025

View reviewed changes

grarco mentioned this pull request Jan 21, 2025

Better management of disposable keys in wallet #4040

Open

grarco reviewed Jan 21, 2025

View reviewed changes

sug0 requested changes Jan 23, 2025

View reviewed changes

grarco requested changes Jan 23, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support setting a shielded address as refund target #4233

Support setting a shielded address as refund target #4233

yito88 commented Jan 13, 2025 •

edited

Loading

codecov bot commented Jan 13, 2025

grarco Jan 21, 2025

grarco Jan 21, 2025

grarco Jan 21, 2025

grarco commented Jan 21, 2025 •

edited

Loading

sug0 Jan 20, 2025

sug0 Jan 20, 2025

sug0 Jan 20, 2025

sug0 Jan 20, 2025

sug0 Jan 20, 2025

yito88 Jan 23, 2025

sug0 Jan 24, 2025 •

edited

Loading

sug0 Jan 20, 2025

sug0 Jan 20, 2025

sug0 Jan 20, 2025

sug0 Jan 20, 2025

grarco Jan 21, 2025

yito88 Jan 23, 2025 •

edited

Loading

grarco Jan 21, 2025

grarco Jan 21, 2025

grarco Jan 21, 2025

grarco Jan 21, 2025

		// Shielded refund only happens if MsgAcknowledgement or MsgTimeout is
		// successful

		fn try_get_refund_masp_tx<Transfer: BorshDeserialize>(
		storage: &S,

	Some(IbcMessage::Envelope(envelope)) => match *envelope {
	// These events are emitted on the receiver
	Some(IbcMessage::Envelope(envelope)) => match *envelope {

		// If there is a transfer to the IBC account, then deduplicate the
		// balance increase since we already account for it below

		let token = convert_to_address(ibc_trace).into_storage_result()?;
		let delta = ValueSum::from_pair(token, amount);

Support setting a shielded address as refund target #4233

Are you sure you want to change the base?

Support setting a shielded address as refund target #4233

Conversation

yito88 commented Jan 13, 2025 • edited Loading

Describe your changes

Checklist before merging

codecov bot commented Jan 13, 2025

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grarco commented Jan 21, 2025 • edited Loading

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

sug0 Jan 24, 2025 • edited Loading

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

yito88 Jan 23, 2025 • edited Loading

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

yito88 commented Jan 13, 2025 •

edited

Loading

grarco commented Jan 21, 2025 •

edited

Loading

sug0 Jan 24, 2025 •

edited

Loading

yito88 Jan 23, 2025 •

edited

Loading