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

fix(minor-interchain-token-service): skip invariant checks for p2p #687

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+209 −41
Open

fix(minor-interchain-token-service): skip invariant checks for p2p #687

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
10bed31
fix(minor-interchain-token-service): skip invariant checks for p2p
cjcobb23 Nov 12, 2024
16f8aec
review
cjcobb23 Nov 12, 2024
e9a08da
cleanup
cjcobb23 Nov 12, 2024
9c64ee9
rm comment
cjcobb23 Nov 12, 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
44 changes: 35 additions & 9 deletions contracts/interchain-token-service/src/contract/execute/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use axelar_core_std::nexus;
use axelar_wasm_std::{killswitch, nonempty, FnExt, IntoContractError};
use cosmwasm_std::{DepsMut, HexBinary, QuerierWrapper, Response, Storage};
use error_stack::{bail, ensure, report, Result, ResultExt};
Expand Down Expand Up @@ -72,6 +73,8 @@
token_id: TokenId,
chain: ChainNameRaw,
},
#[error("error querying nexus module for chain registration for chain {0}")]
Nexus(ChainNameRaw),
}

/// Executes an incoming ITS message.
Expand Down Expand Up @@ -111,6 +114,7 @@

let message = apply_to_hub(
deps.storage,
deps.querier,
cc_id.source_chain.clone(),
destination_chain.clone(),
message,
Expand Down Expand Up @@ -141,6 +145,7 @@

fn apply_to_hub(
storage: &mut dyn Storage,
querier: QuerierWrapper,
source_chain: ChainNameRaw,
destination_chain: ChainNameRaw,
message: Message,
Expand All @@ -150,7 +155,7 @@

match message {
Message::InterchainTransfer(transfer) => {
apply_to_transfer(storage, source_chain, destination_chain, transfer)
apply_to_transfer(storage, querier, source_chain, destination_chain, transfer)
.map(Message::InterchainTransfer)?
}
Message::DeployInterchainToken(deploy_token) => {
Expand All @@ -163,18 +168,39 @@

fn apply_to_transfer(
storage: &mut dyn Storage,
querier: QuerierWrapper,
source_chain: ChainNameRaw,
destination_chain: ChainNameRaw,
transfer: InterchainTransfer,
) -> Result<InterchainTransfer, Error> {
interceptors::subtract_supply_amount(storage, &source_chain, &transfer)?;
let transfer = interceptors::apply_scaling_factor_to_amount(
storage,
&source_chain,
&destination_chain,
transfer,
)?;
interceptors::add_supply_amount(storage, &destination_chain, &transfer)?;
// we only do balance tracking for amplifier chains
let client: nexus::Client = client::CosmosClient::new(querier).into();
let source_is_amplifier_chain = !client
.is_chain_registered(&source_chain.normalize())
.change_context(Error::Nexus(source_chain.clone()))?;
let destination_is_amplifier_chain = !client
.is_chain_registered(&destination_chain.normalize())
.change_context(Error::Nexus(destination_chain.clone()))?;

if source_is_amplifier_chain {
interceptors::subtract_supply_amount(storage, &source_chain, &transfer)?;
}
Copy link
Collaborator

@fish-sammy fish-sammy Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is very procedural and is exactly what the interceptors PR wanted to avoid. I'd much prefer this function to just look like

    interceptors::subtract_supply_amount_if_amplifier(storage, querier, &source_chain, &transfer)?;
    let transfer = interceptors::apply_scaling_factor_to_amount_if_amplifier(
        storage,
        querier,
        &source_chain,
        &destination_chain,
        transfer,
    )?;
    interceptors::add_supply_amount_if_amplifier(storage, querier, &destination_chain, &transfer)?;

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 don't think we should pass the querier in, because then we are going to need to repeatedly query whether a chain is registered with core. I can just query first and pass that data in though

Copy link
Collaborator

@fish-sammy fish-sammy Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

With the ChainSpecifier, it looks even worse now. We are repeatedly loading/querying/saving a lot of stuff. If gas cost is the top priority, we wouldn't be loading source token instance, subtracting, saving, loading destination token instance, adding, saving, loading both token instances again, applying scaling factor.


// we only possibly scale if both source chain and destination chain are amplifier chains
Copy link
Member

Choose a reason for hiding this comment

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

We still need to scale for consensus chains. E.g. Ethereum <-> Sui

Instead of skipping the interceptors completely for consensus chains, we need to apply the interceptors, but if the token instance can't be loaded and the chain is consensus, then we will need to use the "default" instance instead of failing. Perhaps the default instance can be the Untracked supply and the decimals from the token instance for the origin chain.

This is more convoluted though. We could consider exposing a setter for the token instance for consensus chain and just migrate a few popular tokens but the others would fail

let transfer = if source_is_amplifier_chain && destination_is_amplifier_chain {
interceptors::apply_scaling_factor_to_amount(
storage,
&source_chain,
&destination_chain,
transfer,
)?
} else {
transfer
};

if destination_is_amplifier_chain {
interceptors::add_supply_amount(storage, &destination_chain, &transfer)?;
}

Check warning on line 203 in contracts/interchain-token-service/src/contract/execute/mod.rs

View check run for this annotation

Codecov / codecov/patch

contracts/interchain-token-service/src/contract/execute/mod.rs#L203 

Added line #L203 was not covered by tests

Ok(transfer)
}
Expand Down
59 changes: 59 additions & 0 deletions contracts/interchain-token-service/tests/execute.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ use interchain_token_service::{
};
use router_api::{Address, ChainName, ChainNameRaw, CrossChainId};
use serde_json::json;
use utils::params::{CORE_CHAIN, CORE_ITS_CONTRACT};
use utils::{params, register_chains, TestMessage};

mod utils;
Expand Down Expand Up @@ -950,6 +951,64 @@ fn deploy_interchain_token_tracks_supply() {
);
}

#[test]
fn transfer_interchain_token_from_core_does_not_check_balance_or_deployment() {
let (
mut deps,
TestMessage {
router_message,
source_its_chain: _,
source_its_contract,
destination_its_chain,
destination_its_contract: _,
hub_message,
},
) = utils::setup();

let token_id = hub_message.token_id();
let amount = nonempty::Uint256::try_from(400u64).unwrap();

// this deploys the token from source_its_chain to destination_its_chain
assert_ok!(utils::execute_hub_message(
deps.as_mut(),
router_message.cc_id.clone(),
source_its_contract.clone(),
hub_message,
));

let msg = HubMessage::SendToHub {
destination_chain: destination_its_chain.clone(),
message: InterchainTransfer {
token_id,
source_address: HexBinary::from([1; 32]).try_into().unwrap(),
destination_address: HexBinary::from([2; 32]).try_into().unwrap(),
amount,
data: None,
}
.into(),
};
assert_ok!(utils::execute_hub_message(
deps.as_mut(),
CrossChainId {
source_chain: CORE_CHAIN.parse().unwrap(),
message_id: router_message.cc_id.message_id.clone()
},
CORE_ITS_CONTRACT.parse().unwrap(),
msg,
));

assert_eq!(
assert_ok!(utils::query_token_instance(
deps.as_ref(),
destination_its_chain.clone(),
token_id
))
.unwrap()
.supply,
TokenSupply::Tracked(amount.into())
);
}

#[test]
fn deploy_interchain_token_with_minter_does_not_track_supply() {
let (
Expand Down
12 changes: 11 additions & 1 deletion contracts/interchain-token-service/tests/utils/execute.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ use interchain_token_service::msg::{self, ExecuteMsg, TruncationConfig};
use interchain_token_service::{contract, HubMessage};
use router_api::{Address, ChainName, ChainNameRaw, CrossChainId};

use super::params::{CORE_CHAIN, CORE_ITS_CONTRACT};
use super::{instantiate_contract, TestMessage};
use crate::utils::params;

Expand Down Expand Up @@ -70,7 +71,7 @@ pub fn make_deps() -> OwnedDeps<MemoryStorage, MockApi, MockQuerier<AxelarQueryM
AxelarQueryMsg::Nexus(nexus::query::QueryMsg::IsChainRegistered { chain }) => {
Ok(to_json_binary(
&(IsChainRegisteredResponse {
is_registered: chain == "ethereum",
is_registered: chain == CORE_CHAIN,
}),
)
.into())
Expand Down Expand Up @@ -204,5 +205,14 @@ pub fn setup() -> (
)
.unwrap();

register_chain(
deps.as_mut(),
CORE_CHAIN.parse().unwrap(),
CORE_ITS_CONTRACT.parse().unwrap(),
Uint256::MAX.try_into().unwrap(),
u8::MAX,
)
.unwrap();

(deps, TestMessage::dummy())
}
2 changes: 2 additions & 0 deletions contracts/interchain-token-service/tests/utils/params.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,5 @@ pub const ROUTER: &str = "router";
pub const GATEWAY: &str = "gateway";
pub const GOVERNANCE: &str = "governance";
pub const ADMIN: &str = "admin";
pub const CORE_CHAIN: &str = "core-ethereum";
pub const CORE_ITS_CONTRACT: &str = "core-its-contract";
Loading