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

Ics20 same ack handling as ibctransfer #653

Merged
merged 4 commits into from
Feb 9, 2022
Merged

Ics20 same ack handling as ibctransfer #653

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
13464aa
Update balance in transfer, reduce on_packet_failure, like ibctransfer
ethanfrey Feb 8, 2022
3696e60
Update tests
ethanfrey Feb 8, 2022
82094c8
Check sent ibc packet in tests
ethanfrey Feb 8, 2022
2ea606b
Add version on packet, so we can use old ack handling for in-flight p…
ethanfrey Feb 9, 2022
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
13 changes: 9 additions & 4 deletions contracts/cw20-ics20/src/contract.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,10 @@ use crate::msg::{
AllowMsg, AllowedInfo, AllowedResponse, ChannelResponse, ConfigResponse, ExecuteMsg, InitMsg,
ListAllowedResponse, ListChannelsResponse, MigrateMsg, PortResponse, QueryMsg, TransferMsg,
};
use crate::state::{AllowInfo, Config, ADMIN, ALLOW_LIST, CHANNEL_INFO, CHANNEL_STATE, CONFIG};
use crate::state::{
increase_channel_balance, AllowInfo, Config, ADMIN, ALLOW_LIST, CHANNEL_INFO, CHANNEL_STATE,
CONFIG,
};
use cw_utils::{maybe_addr, nonpayable, one_coin};

// version info for migration info
Expand Down Expand Up @@ -130,16 +133,18 @@ pub fn execute_transfer(
);
packet.validate()?;

// Update the balance now (optimistically) like ibctransfer modules.
// In on_packet_failure (ack with error message or a timeout), we reduce the balance appropriately.
// This means the channel works fine if success acks are not relayed.
increase_channel_balance(deps.storage, &msg.channel, &amount.denom(), amount.amount())?;

// prepare ibc message
let msg = IbcMsg::SendPacket {
channel_id: msg.channel,
data: to_binary(&packet)?,
timeout: timeout.into(),
};

// Note: we update local state when we get ack - do not count this transfer towards anything until acked
// similar event messages like ibctransfer module

// send response
let res = Response::new()
.add_message(msg)
Expand Down
104 changes: 64 additions & 40 deletions contracts/cw20-ics20/src/ibc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,15 +32,20 @@ pub struct Ics20Packet {
pub receiver: String,
/// the sender address
pub sender: String,
/// used only by us to control ack handling
pub v: Option<u32>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I created an issue about this #662

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I found this and had to revert it.
Turns out ibctransfer packet uses a super-strict JSON parser that fails on any unknown fields.
It seems to me that this is undesired behavior, but that is another discussion.

I need a different format and migration step to do a new release of this, very correct. I was out for a while.

}

const V2: u32 = 2;

impl Ics20Packet {
pub fn new<T: Into<String>>(amount: Uint128, denom: T, sender: &str, receiver: &str) -> Self {
Ics20Packet {
denom: denom.into(),
amount,
sender: sender.to_string(),
receiver: receiver.to_string(),
v: Some(V2),
}
}

Expand Down Expand Up @@ -314,6 +319,13 @@ pub fn ibc_packet_timeout(
// update the balance stored on this (channel, denom) index
fn on_packet_success(deps: DepsMut, packet: IbcPacket) -> Result<IbcBasicResponse, ContractError> {
let msg: Ics20Packet = from_binary(&packet.data)?;

// if this was for an older (pre-v2) packet we send continue with old behavior
// (this is needed for transitioning on a system with pending packet)
if msg.v.is_none() {
increase_channel_balance(deps.storage, &packet.src.channel_id, &msg.denom, msg.amount)?;
}

// similar event messages like ibctransfer module
let attributes = vec![
attr("action", "acknowledge"),
Expand All @@ -324,9 +336,6 @@ fn on_packet_success(deps: DepsMut, packet: IbcPacket) -> Result<IbcBasicRespons
attr("success", "true"),
];

// we have made a proper transfer, record this
maurolacy marked this conversation as resolved.
Show resolved Hide resolved
increase_channel_balance(deps.storage, &packet.src.channel_id, &msg.denom, msg.amount)?;

Ok(IbcBasicResponse::new().add_attributes(attributes))
}

Expand All @@ -338,6 +347,11 @@ fn on_packet_failure(
) -> Result<IbcBasicResponse, ContractError> {
let msg: Ics20Packet = from_binary(&packet.data)?;

// undo the balance update (but not for pre-v2/None packets which didn't add before sending)
if msg.v.is_some() {
reduce_channel_balance(deps.storage, &packet.src.channel_id, &msg.denom, msg.amount)?;
}

let to_send = Amount::from_parts(msg.denom.clone(), msg.amount);
let gas_limit = check_gas_limit(deps.as_ref(), &to_send)?;
let send = send_amount(to_send, msg.sender.clone());
Expand Down Expand Up @@ -385,9 +399,11 @@ mod test {
use super::*;
use crate::test_helpers::*;

use crate::contract::query_channel;
use cosmwasm_std::testing::mock_env;
use cosmwasm_std::{coins, to_vec, IbcAcknowledgement, IbcEndpoint, IbcTimeout, Timestamp};
use crate::contract::{execute, query_channel};
use crate::msg::{ExecuteMsg, TransferMsg};
use cosmwasm_std::testing::{mock_env, mock_info};
use cosmwasm_std::{coins, to_vec, IbcEndpoint, IbcMsg, IbcTimeout, Timestamp};
use cw20::Cw20ReceiveMsg;

#[test]
fn check_ack_json() {
Expand All @@ -410,7 +426,7 @@ mod test {
"wasm1fucynrfkrt684pm8jrt8la5h2csvs5cnldcgqc",
);
// Example message generated from the SDK
let expected = r#"{"amount":"12345","denom":"ucosm","receiver":"wasm1fucynrfkrt684pm8jrt8la5h2csvs5cnldcgqc","sender":"cosmos1zedxv25ah8fksmg2lzrndrpkvsjqgk4zt5ff7n"}"#;
let expected = r#"{"amount":"12345","denom":"ucosm","receiver":"wasm1fucynrfkrt684pm8jrt8la5h2csvs5cnldcgqc","sender":"cosmos1zedxv25ah8fksmg2lzrndrpkvsjqgk4zt5ff7n","v":2}"#;

let encdoded = String::from_utf8(to_vec(&packet).unwrap()).unwrap();
assert_eq!(expected, encdoded.as_str());
Expand Down Expand Up @@ -446,27 +462,6 @@ mod test {
)
}

fn mock_sent_packet(my_channel: &str, amount: u128, denom: &str, sender: &str) -> IbcPacket {
let data = Ics20Packet {
denom: denom.into(),
amount: amount.into(),
sender: sender.to_string(),
receiver: "remote-rcpt".to_string(),
};
IbcPacket::new(
to_binary(&data).unwrap(),
IbcEndpoint {
port_id: CONTRACT_PORT.to_string(),
channel_id: my_channel.to_string(),
},
IbcEndpoint {
port_id: REMOTE_PORT.to_string(),
channel_id: "channel-1234".to_string(),
},
2,
IbcTimeout::with_timestamp(Timestamp::from_seconds(1665321069)),
)
}
fn mock_receive_packet(
my_channel: &str,
amount: u128,
Expand All @@ -479,6 +474,7 @@ mod test {
amount: amount.into(),
sender: "remote-sender".to_string(),
receiver: receiver.to_string(),
v: Some(V2),
};
print!("Packet denom: {}", &data.denom);
IbcPacket::new(
Expand Down Expand Up @@ -508,23 +504,48 @@ mod test {
);

// prepare some mock packets
let sent_packet = mock_sent_packet(send_channel, 987654321, cw20_denom, "local-sender");
let recv_packet = mock_receive_packet(send_channel, 876543210, cw20_denom, "local-rcpt");
let recv_high_packet =
mock_receive_packet(send_channel, 1876543210, cw20_denom, "local-rcpt");

let msg = IbcPacketReceiveMsg::new(recv_packet.clone());
// cannot receive this denom yet
let msg = IbcPacketReceiveMsg::new(recv_packet.clone());
let res = ibc_packet_receive(deps.as_mut(), mock_env(), msg).unwrap();
assert!(res.messages.is_empty());
let ack: Ics20Ack = from_binary(&res.acknowledgement).unwrap();
let no_funds = Ics20Ack::Error(ContractError::InsufficientFunds {}.to_string());
assert_eq!(ack, no_funds);

// we get a success cache (ack) for a send
let msg = IbcPacketAckMsg::new(IbcAcknowledgement::new(ack_success()), sent_packet);
let res = ibc_packet_ack(deps.as_mut(), mock_env(), msg).unwrap();
assert_eq!(0, res.messages.len());
// we send some cw20 tokens over
let transfer = TransferMsg {
channel: send_channel.to_string(),
remote_address: "remote-rcpt".to_string(),
timeout: None,
};
let msg = ExecuteMsg::Receive(Cw20ReceiveMsg {
sender: "local-sender".to_string(),
amount: Uint128::new(987654321),
msg: to_binary(&transfer).unwrap(),
});
let info = mock_info(cw20_addr, &[]);
let res = execute(deps.as_mut(), mock_env(), info, msg).unwrap();
assert_eq!(1, res.messages.len());
let expected = Ics20Packet {
denom: cw20_denom.into(),
amount: Uint128::new(987654321),
sender: "local-sender".to_string(),
receiver: "remote-rcpt".to_string(),
v: Some(V2),
};
let timeout = mock_env().block.time.plus_seconds(DEFAULT_TIMEOUT);
assert_eq!(
&res.messages[0],
&SubMsg::new(IbcMsg::SendPacket {
channel_id: send_channel.to_string(),
data: to_binary(&expected).unwrap(),
timeout: IbcTimeout::with_timestamp(timeout),
})
);

// query channel state|_|
let state = query_channel(deps.as_ref(), send_channel.to_string()).unwrap();
Expand Down Expand Up @@ -565,7 +586,6 @@ mod test {
let denom = "uatom";

// prepare some mock packets
let sent_packet = mock_sent_packet(send_channel, 987654321, denom, "local-sender");
let recv_packet = mock_receive_packet(send_channel, 876543210, denom, "local-rcpt");
let recv_high_packet = mock_receive_packet(send_channel, 1876543210, denom, "local-rcpt");

Expand All @@ -577,10 +597,14 @@ mod test {
let no_funds = Ics20Ack::Error(ContractError::InsufficientFunds {}.to_string());
assert_eq!(ack, no_funds);

// we get a success cache (ack) for a send
let msg = IbcPacketAckMsg::new(IbcAcknowledgement::new(ack_success()), sent_packet);
let res = ibc_packet_ack(deps.as_mut(), mock_env(), msg).unwrap();
assert_eq!(0, res.messages.len());
// we transfer some tokens
let msg = ExecuteMsg::Transfer(TransferMsg {
channel: send_channel.to_string(),
remote_address: "my-remote-address".to_string(),
timeout: None,
});
let info = mock_info("local-sender", &coins(987654321, denom));
execute(deps.as_mut(), mock_env(), info, msg).unwrap();

// query channel state|_|
let state = query_channel(deps.as_ref(), send_channel.to_string()).unwrap();
Expand All @@ -605,7 +629,7 @@ mod test {
let ack: Ics20Ack = from_binary(&res.acknowledgement).unwrap();
matches!(ack, Ics20Ack::Result(_));

// TODO: we must call the reply block
// only need to call reply block on error case

// query channel state
let state = query_channel(deps.as_ref(), send_channel.to_string()).unwrap();
Expand Down