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.

Sign up for GitHub

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

Jump to bottom

Receive a token to a payment address over IBC #1917

Merged
merged 14 commits into from
Oct 24, 2023
Merged

Receive a token to a payment address over IBC #1917

merged 14 commits into from
Oct 24, 2023

Conversation

yito88
Copy link
Member

@yito88 yito88 commented Sep 21, 2023 •
edited
Loading

Describe your changes

The sender needs to do two operations to send a token to the payment address on the destination Namada

  1. Request to generate MASP transaction with ibc-gen-shielded command on the destination Namada
    • Need not only the receiver(payment address), the token and the amount, but also port/channel IDs that are used in the following IBC transfer
    • The command makes a file including the serialized memo for the IBC transfer
  2. Set the MASP transfer to the memo field of MsgTransfer
    • with the option --memo-path to specify the memo file

Indicate on which release or other PRs this topic is based on

Based on #1951

Checklist before merging to draft

  • I have added a changelog
  • Git history is in acceptable state

@yito88 yito88 requested a review from sug0 September 21, 2023 15:28
sug0
sug0 requested changes Sep 22, 2023
View reviewed changes
shared/src/ledger/vp_host_fns.rs Outdated Show resolved Hide resolved
core/src/types/ibc.rs Outdated Show resolved Hide resolved
core/src/ledger/vp_env.rs Outdated Show resolved Hide resolved
core/src/ledger/vp_env.rs Outdated Show resolved Hide resolved
core/src/ledger/ibc/mod.rs Outdated Show resolved Hide resolved
core/src/ledger/ibc/mod.rs Outdated Show resolved Hide resolved
core/src/types/address.rs Outdated Show resolved Hide resolved
core/src/types/address.rs Outdated Show resolved Hide resolved
shared/src/ledger/protocol/mod.rs Outdated Show resolved Hide resolved
shared/src/ledger/tx.rs Outdated Show resolved Hide resolved
@sug0
Copy link
Contributor

sug0 commented Sep 22, 2023

actually, can you clarify why we're introducing foreign addresses please? @yito88

@yito88
Copy link
Member Author

yito88 commented Sep 22, 2023

Address::Foreign wraps the sender/receiver address on the source/destination chain to set them in token::Transfer or MASP tx generation. Basically, Namada doesn't know the addresses.
Anyway, as you mentioned, I have to refine the handling.

@yito88
Copy link
Member Author

yito88 commented Sep 29, 2023

I rebased this branch based on #1951.
And, the sender address for a shielded transfer isn't required. So, I remove Address::Foreign and use InternalAddress::Ibc.

@yito88 yito88 requested a review from sug0 October 4, 2023 20:47
@yito88 yito88 marked this pull request as ready for review October 6, 2023 12:14
sug0
sug0 reviewed Oct 9, 2023
View reviewed changes
Copy link
Contributor

@sug0 sug0 left a comment

Choose a reason for hiding this comment

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

lgtm. some tinies

shared/src/sdk/tx.rs Outdated Show resolved Hide resolved
shared/src/sdk/tx.rs Outdated Show resolved Hide resolved
@sug0 sug0 self-requested a review October 9, 2023 16:10
@sug0 sug0 requested a review from tzemanovic October 9, 2023 16:11
@Fraccaman Fraccaman mentioned this pull request Oct 16, 2023
Closed
yito88 added a commit that referenced this pull request Oct 17, 2023
@yito88
Merge branch 'yuji/ibc-shielded-actions' (#1917)
4262206
@cwgoes cwgoes mentioned this pull request Oct 20, 2023
Closed
23 tasks
Fraccaman added a commit that referenced this pull request Oct 23, 2023
@Fraccaman
Merge branch 'yuji/ibc-shielded-actions' (#1917)
dde4ed2 
* origin/yuji/ibc-shielded-actions:
  fix error handling
  add changelog
  fix get_ibc_event
  remove Address::Foreign
  fix get_shielded_action
  fix after merge
  workaround for decoding asset types for IbcToken
  WIP: add e2e test for receiving to payment address
  add gen_ibc_shielded_transfer
  WIP: receive to a shielded address
tzemanovic added a commit that referenced this pull request Oct 24, 2023
@tzemanovic
Merge branch 'yuji/ibc-shielded-actions' (#1917)
97c31d3 
* origin/yuji/ibc-shielded-actions:
  fix error handling
  add changelog
  fix get_ibc_event
  remove Address::Foreign
  fix get_shielded_action
  fix after merge
  workaround for decoding asset types for IbcToken
  WIP: add e2e test for receiving to payment address
  add gen_ibc_shielded_transfer
  WIP: receive to a shielded address
@tzemanovic tzemanovic mentioned this pull request Oct 24, 2023
Merged
@tzemanovic tzemanovic merged commit af011f6 into main Oct 24, 2023
11 of 14 checks passed
@tzemanovic tzemanovic deleted the yuji/ibc-shielded-actions branch October 24, 2023 11:26
brentstone pushed a commit that referenced this pull request Nov 11, 2023
@tzemanovic @brentstone
Merge branch 'yuji/ibc-shielded-actions' (#1917)
d26dc08 
* origin/yuji/ibc-shielded-actions:
  fix error handling
  add changelog
  fix get_ibc_event
  remove Address::Foreign
  fix get_shielded_action
  fix after merge
  workaround for decoding asset types for IbcToken
  WIP: add e2e test for receiving to payment address
  add gen_ibc_shielded_transfer
  WIP: receive to a shielded address
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sug0 sug0 sug0 approved these changes

@tzemanovic tzemanovic tzemanovic approved these changes

Assignees
No one assigned
Labels
IBC
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yito88 @sug0 @tzemanovic