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

feat: Cross Chain Rates Recipient #671

Merged
merged 9 commits into from
Nov 27, 2024
Merged

feat: Cross Chain Rates Recipient #671

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d94100e
feat: handle cross chain recipient in rates
joemonem Nov 25, 2024
55a94a9
test: fix test_kernel_ibc_funds_only
joemonem Nov 25, 2024
da74efe
ref: re-enable economics in cw721 and auction
joemonem Nov 25, 2024
bb2eb6c
test: add economics to adodb
joemonem Nov 25, 2024
bf33106
test: remove unused economics
joemonem Nov 25, 2024
c95f460
ref: disallow cw20 rates with cross-chain recipients
joemonem Nov 27, 2024
471f6f4
ref: apply previous change to the rates contract
joemonem Nov 27, 2024
040cdfe
ref: simplify rates validation
joemonem Nov 27, 2024
aabea26
ref: ensure native fees for cross chain rates recipient
joemonem Nov 27, 2024
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
2 changes: 1 addition & 1 deletion Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion contracts/os/andromeda-kernel/src/execute.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -774,7 +774,6 @@ impl MsgHandler {
gas_limit: None,
reply_on: cosmwasm_std::ReplyOn::Always,
});

Ok(resp
.add_attribute(format!("method:{sequence}"), "execute_transfer_funds")
.add_attribute(format!("channel:{sequence}"), channel)
Expand Down
2 changes: 1 addition & 1 deletion packages/std/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "andromeda-std"
version = "1.4.0"
version = "1.5.0"
edition = "2021"
rust-version = "1.75.0"
description = "The standard library for creating an Andromeda Digital Object"
Expand Down
62 changes: 47 additions & 15 deletions packages/std/src/ado_base/rates.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,17 @@
use crate::{
ado_contract::ADOContract,
amp::{AndrAddr, Recipient},
amp::{
messages::{AMPMsg, AMPMsgConfig},
AndrAddr, Recipient,
},
common::{deduct_funds, denom::validate_native_denom, Funds},
error::ContractError,
os::{adodb::ADOVersion, aos_querier::AOSQuerier},
};
use cosmwasm_schema::cw_serde;
use cosmwasm_std::{
ensure, has_coins, to_json_binary, Coin, Decimal, Deps, Event, Fraction, QueryRequest, SubMsg,
WasmQuery,
ensure, has_coins, to_json_binary, Addr, Coin, Decimal, Deps, Event, Fraction, QueryRequest,
ReplyOn, SubMsg, WasmMsg, WasmQuery,
};
use cw20::{Cw20Coin, Cw20QueryMsg, TokenInfoResponse};

Expand Down Expand Up @@ -160,24 +163,53 @@ impl LocalRate {
event = event.add_attribute(
"payment",
PaymentAttribute {
receiver: self.recipient.address.get_raw_address(&deps)?.to_string(),
receiver: self
.recipient
.address
.get_raw_address(&deps)
.unwrap_or(Addr::unchecked(self.recipient.address.to_string()))
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason to generate unchecked Addr in case get_raw_address failed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm assuming if it fails then it's a cross-chain address. Maybe we should account for that possibility in get_raw_address itself? Feels like a topic worth discussing.
@crnbarr93

.to_string(),
Comment on lines +195 to +197
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle potential errors when retrieving recipient's raw address

Using unwrap_or with Addr::unchecked may mask errors from get_raw_address, potentially leading to invalid addresses being used without proper validation. It's important to handle the Result returned by get_raw_address explicitly to avoid unexpected behavior.

Apply this diff to handle the error properly:

-        .get_raw_address(&deps)
-        .unwrap_or(Addr::unchecked(self.recipient.address.to_string()))
-        .to_string(),
+        .get_raw_address(&deps)?
+        .to_string(),

This change will propagate any errors from get_raw_address, ensuring that invalid addresses are not silently accepted.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
.get_raw_address(&deps)
.unwrap_or(Addr::unchecked(self.recipient.address.to_string()))
.to_string(),
.get_raw_address(&deps)?
.to_string(),

amount: fee.clone(),
}
.to_string(),
);

let msg = if is_native {
self.recipient
.generate_direct_msg(&deps, vec![fee.clone()])?
} else {
self.recipient.generate_msg_cw20(
&deps,
Cw20Coin {
amount: fee.amount,
address: fee.denom.to_string(),
let msg = if self.recipient.is_cross_chain() {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should ensure cw20 isn't used for cross-chain here, maybe at the point of creating the rates recipient we validate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would work when the LocalRateValue is flat, but when it's a percentage we'll have to handle it while processing the rate right?

// Create a cross chain message to be sent to the kernel
let kernel_address = ADOContract::default().get_kernel_address(deps.storage)?;
let kernel_msg = crate::os::kernel::ExecuteMsg::Send {
message: AMPMsg {
recipient: self.recipient.address.clone(),
message: self.recipient.msg.clone().unwrap_or_default(),
funds: vec![fee.clone()],
config: AMPMsgConfig {
reply_on: ReplyOn::Always,
exit_at_error: false,
gas_limit: None,
direct: true,
ibc_config: None,
},
},
)?
};
SubMsg::new(WasmMsg::Execute {
contract_addr: kernel_address.to_string(),
msg: to_json_binary(&kernel_msg)?,
funds: vec![fee.clone()],
})
} else {
if is_native {
self.recipient
.generate_direct_msg(&deps, vec![fee.clone()])?
} else {
self.recipient.generate_msg_cw20(
&deps,
Cw20Coin {
amount: fee.amount,
address: fee.denom.to_string(),
},
)?
}
};

msgs.push(msg);

events.push(event);
Expand Down
8 changes: 8 additions & 0 deletions packages/std/src/amp/recipient.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,14 @@ impl Recipient {
self.msg.clone()
}

pub fn is_cross_chain(&self) -> bool {
let protocol = self.address.get_protocol();
match protocol {
Some("ibc") => true,
_ => false,
}
}
Comment on lines +61 to +67
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add test coverage for is_cross_chain method.

The new method lacks test coverage. Please add tests to verify:

  1. IBC protocol addresses are correctly identified
  2. Non-IBC protocol addresses return false
  3. Local addresses (no protocol) return false
#[test]
fn test_is_cross_chain() {
    // IBC address should return true
    let recipient = Recipient::from_string("ibc/transfer/channel-0/uatom");
    assert!(recipient.is_cross_chain());

    // Non-IBC protocol should return false
    let recipient = Recipient::from_string("wasm/contract0x123");
    assert!(!recipient.is_cross_chain());

    // Local address should return false
    let recipient = Recipient::from_string("local_address");
    assert!(!recipient.is_cross_chain());
}


/// Generates a direct sub message for the given recipient.
pub fn generate_direct_msg(
&self,
Expand Down
Loading
Loading