forked from Sovereign-Labs/sovereign-sdk
-
Notifications
You must be signed in to change notification settings - Fork 0
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
Fix send_transfer
#2
Merged
Merged
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
5100dd1
Remove old struct
plafer 94accde
WIP send_transfer fix
plafer df6edc2
Add public function for fetching token names
preston-evans98 c92e3aa
fix non-uniqueness of some denoms
plafer 6de7c7b
`minted_tokens` is now indexed by denom
plafer 4af00cc
Add test coverage
preston-evans98 63fec6c
`escrowed_tokens` now uses denom as index
plafer 7a2d118
lint
preston-evans98 3d119b0
Merge branch 'plafer/ibc-integration' into plafer/send_transfer_fix
plafer 8e741bd
Merge branch 'plafer/ibc-integration' into plafer/send_transfer_fix
plafer 1f891bf
Merge remote-tracking branch 'upstream/preston/ibc-token-name' into p…
plafer bd2b811
Get real token address
plafer 7821856
fix `transfer()`
plafer 5155b5f
fix lack of amount in SdkTokenTransfer
plafer 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
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
131 changes: 131 additions & 0 deletions
131
module-system/module-implementations/sov-ibc-transfer/src/call.rs
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,131 @@ | ||
use std::cell::RefCell; | ||
use std::rc::Rc; | ||
|
||
use anyhow::Result; | ||
use ibc::applications::transfer::context::TokenTransferExecutionContext; | ||
use ibc::applications::transfer::msgs::transfer::MsgTransfer; | ||
use ibc::applications::transfer::packet::PacketData; | ||
use ibc::applications::transfer::{send_transfer, Memo, PrefixedCoin}; | ||
use ibc::core::ics04_channel::timeout::TimeoutHeight; | ||
use ibc::core::ics24_host::identifier::{ChannelId, PortId}; | ||
use ibc::core::timestamp::Timestamp; | ||
use ibc::core::ExecutionContext; | ||
use ibc::Signer; | ||
use sov_state::WorkingSet; | ||
|
||
use crate::context::EscrowExtraData; | ||
use crate::Transfer; | ||
|
||
#[derive(borsh::BorshDeserialize, borsh::BorshSerialize, Debug, PartialEq)] | ||
pub struct SDKTokenTransfer<C: sov_modules_api::Context> { | ||
/// the port on which the packet will be sent | ||
pub port_id_on_a: PortId, | ||
/// the channel by which the packet will be sent | ||
pub chan_id_on_a: ChannelId, | ||
/// Timeout height relative to the current block height. | ||
/// The timeout is disabled when set to None. | ||
pub timeout_height_on_b: TimeoutHeight, | ||
/// Timeout timestamp relative to the current block timestamp. | ||
/// The timeout is disabled when set to 0. | ||
pub timeout_timestamp_on_b: Timestamp, | ||
/// The address of the token to be sent | ||
pub token_address: C::Address, | ||
/// The amount of tokens sent | ||
pub amount: sov_bank::Amount, | ||
/// The address of the token sender | ||
pub sender: Signer, | ||
/// The address of the token receiver on the counterparty chain | ||
pub receiver: Signer, | ||
/// Additional note associated with the message | ||
pub memo: Memo, | ||
} | ||
|
||
impl<C> Transfer<C> | ||
where | ||
C: sov_modules_api::Context, | ||
{ | ||
pub fn transfer( | ||
&self, | ||
sdk_token_transfer: SDKTokenTransfer<C>, | ||
execution_context: &mut impl ExecutionContext, | ||
token_ctx: &mut impl TokenTransferExecutionContext<EscrowExtraData<C>>, | ||
working_set: Rc<RefCell<&mut WorkingSet<C::Storage>>>, | ||
) -> Result<sov_modules_api::CallResponse> { | ||
let msg_transfer: MsgTransfer = { | ||
let denom = { | ||
let token_name = self | ||
.bank | ||
.get_token_name( | ||
&sdk_token_transfer.token_address, | ||
&mut working_set.borrow_mut(), | ||
) | ||
.ok_or(anyhow::anyhow!( | ||
"Token with address {} doesn't exist", | ||
sdk_token_transfer.token_address | ||
))?; | ||
|
||
if self.token_was_created_by_ibc( | ||
&token_name, | ||
&sdk_token_transfer.token_address, | ||
&mut working_set.borrow_mut(), | ||
) { | ||
// The token was created by the IBC module, and the ICS-20 | ||
// denom was stored in the token name. Hence, we need to use | ||
// the token name as denom. | ||
token_name | ||
} else { | ||
// This applies to all other tokens created on this | ||
// sovereign SDK chain. The token name is not guaranteed to | ||
// be unique, and hence we must use the token address (which | ||
// is guaranteed to be unique) as the ICS-20 denom to ensure | ||
// uniqueness. | ||
sdk_token_transfer.token_address.to_string() | ||
} | ||
Farhad-Shabani marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}; | ||
|
||
MsgTransfer { | ||
port_id_on_a: sdk_token_transfer.port_id_on_a, | ||
chan_id_on_a: sdk_token_transfer.chan_id_on_a, | ||
packet_data: PacketData { | ||
token: PrefixedCoin { | ||
denom: denom | ||
.parse() | ||
.map_err(|_err| anyhow::anyhow!("Failed to parse denom {denom}"))?, | ||
amount: sdk_token_transfer.amount.into(), | ||
}, | ||
sender: sdk_token_transfer.sender, | ||
receiver: sdk_token_transfer.receiver, | ||
memo: sdk_token_transfer.memo, | ||
}, | ||
timeout_height_on_b: sdk_token_transfer.timeout_height_on_b, | ||
timeout_timestamp_on_b: sdk_token_transfer.timeout_timestamp_on_b, | ||
} | ||
}; | ||
|
||
send_transfer( | ||
execution_context, | ||
token_ctx, | ||
msg_transfer, | ||
&EscrowExtraData { | ||
token_address: sdk_token_transfer.token_address, | ||
}, | ||
)?; | ||
|
||
todo!() | ||
} | ||
|
||
/// This function returns true if the token to be sent was created by IBC. | ||
/// This only occurs for tokens that are native to and received from other | ||
/// chains; i.e. for tokens for which this chain isn't the source. | ||
fn token_was_created_by_ibc( | ||
&self, | ||
token_name: &str, | ||
token_address: &C::Address, | ||
working_set: &mut WorkingSet<C::Storage>, | ||
) -> bool { | ||
match self.minted_tokens.get(token_name, working_set) { | ||
Some(minted_token_address) => minted_token_address == *token_address, | ||
None => false, | ||
} | ||
} | ||
} |
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
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
Oops, something went wrong.
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.
I am seeing, when transferring tokens between two Sovereign chains, we exclusively use the token address as the denom, disregarding the token's name.
Whether the token lives on the source chain, in which case the denom is based on the token address, or if the IBC mints the token on the source chain (which represents a token on the destination chain). In latter, we use a prefixed denom, where the base denom comes from a
MsgRecvPacket
, which was earlier crafted by relayers based on an emittedTransferEvent
from the destination chain...Where the denom is also set as the token address!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.
In the initial version, we will only support sending tokens to Tendermint-based chains, as we will only have a tendermint light client installed.
However, if we were to support Sovereign <-> Sovereign transfers, then the denom of tokens we receive would indeed represent the address of the token on the other chain. Note that the token we mint on our side would have a different address, but its denom would be set to the string representation of the address of the token on the other chain. However, for received tokens, we treat the token name as a black box; we never try to interpret it in any way.
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.
Exactly. And looks like it holds true for bunch of other non-Cosmos chains, which makes string reps of token addresses the most common denom.
By the way, the transfer module stores the "denom → token_address" in the
minted_token
field. This allows us to map, e.g. thechannel-1/port-1/0xb79…4f5ea
received fromMsgRecvPacket
to the corresponding token address on the source chain. So, I'm still a bit puzzled about why can't we work with theMsgTransfer
and include the string reps of the token's address as denom? What required us to defineSDKTokenTransfer
on top of that?Anyway, as chatted earlier, Let's dive deeper into these details when writing tests.