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

cjcobb23 · 2024-11-12T03:24:00Z

ITS tokens deployed between core chains are deployed in p2p mode, and the hub has no awareness of their deployment. Therefore, we don't track balances for the tokens when the source or destination chain is a core chain, and we don't check whether the token is deployed.

https://axelarnetwork.atlassian.net/browse/AXE-6434

* ITS tokens deployed between core chains are deployed in p2p mode, and the hub has no awareness of their deployment. Therefore, we don't track balances for the tokens when the source or destination chain is a core chain, and we don't check whether the token is deployed.

codecov · 2024-11-12T03:27:04Z

Codecov Report

Attention: Patch coverage is 94.87179% with 6 lines in your changes missing coverage. Please review.

Project coverage is 93.53%. Comparing base (da659f4) to head (7dd4581).

Files with missing lines	Patch %	Lines
...token-service/src/contract/execute/interceptors.rs	93.93%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #687      +/-   ##
==========================================
+ Coverage   93.52%   93.53%   +0.01%     
==========================================
  Files         228      228              
  Lines       34758    34837      +79     
==========================================
+ Hits        32507    32585      +78     
- Misses       2251     2252       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

milapsheth · 2024-11-12T15:40:48Z

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

+        interceptors::subtract_supply_amount(storage, &source_chain, &transfer)?;
+    }
+
+    // we only possibly scale if both source chain and destination chain are amplifier chains


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

fish-sammy · 2024-11-12T17:17:05Z

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

+    // 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)?;
+    }


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)?;

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

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.

fish-sammy · 2024-11-12T21:42:51Z

contracts/interchain-token-service/src/contract/execute/interceptors.rs

+    let res = state::may_load_token_instance(storage, chain.name.clone(), token_id)
+        .change_context(Error::State)?
+        .ok_or(report!(Error::TokenNotDeployed {
+            token_id,
+            chain: chain.name.clone(),
+        }));
+    if res.is_err() && !chain.is_amplifier_chain {
+        return try_load_origin_chain_token_instance(storage, token_id);
+    }
+    res


Suggested change

let res = state::may_load_token_instance(storage, chain.name.clone(), token_id)

.change_context(Error::State)?

.ok_or(report!(Error::TokenNotDeployed {

token_id,

chain: chain.name.clone(),

}));

if res.is_err() && !chain.is_amplifier_chain {

return try_load_origin_chain_token_instance(storage, token_id);

}

res

match state::may_load_token_instance(storage, chain.name.clone(), token_id)

.change_context(Error::State)?

.ok_or(report!(Error::TokenNotDeployed {

token_id,

chain: chain.name.clone(),

})) {

Ok(token) => Ok(token),

Err(_) if !chain.is_amplifier_chain => {

try_load_origin_chain_token_instance(storage, token_id)

}

Err(err) => Err(err),

}

Add a comment saying that the default is from the original chain.

milapsheth · 2024-11-12T22:29:04Z

contracts/interchain-token-service/src/contract/execute/interceptors.rs

+    let res = state::may_load_token_instance(storage, chain.name.clone(), token_id)
+        .change_context(Error::State)?
+        .ok_or(report!(Error::TokenNotDeployed {
+            token_id,
+            chain: chain.name.clone(),
+        }));
+    if res.is_err() && !chain.is_amplifier_chain {
+        return try_load_origin_chain_token_instance(storage, token_id);
+    }
+    res


Suggested change

let res = state::may_load_token_instance(storage, chain.name.clone(), token_id)

.change_context(Error::State)?

.ok_or(report!(Error::TokenNotDeployed {

token_id,

chain: chain.name.clone(),

}));

if res.is_err() && !chain.is_amplifier_chain {

return try_load_origin_chain_token_instance(storage, token_id);

}

res

let Some(token_instance) = state::may_load_token_instance(storage, chain.name.clone(), token_id)

.change_context(Error::State)? {

return token_instance

};

ensure!(!chain.is_amplifier_chain, Error::TokenNotDeployed {

token_id,

chain: chain.name.clone(),

});

try_load_origin_chain_token_instance(storage, token_id)

milapsheth · 2024-11-12T22:32:05Z

contracts/interchain-token-service/src/contract/execute/interceptors.rs

+    chain: &ChainSpecifier,
+    token_id: TokenId,
+) -> Result<TokenInstance, Error> {
+    let res = state::may_load_token_instance(storage, chain.name.clone(), token_id)


Querying here everytime for now will be much cleaner, the additional gas cost isn't a significant concern for now

we can't only query here, we also need to query when doing balance tracking, which shouldn't use this method, since it would then modify the balance for the origin chain

cgorenflo · 2024-12-19T16:26:40Z

We're abandoning this PR in favour of finding a better solution

cjcobb23 requested a review from a team as a code owner November 12, 2024 03:24

milapsheth reviewed Nov 12, 2024

View reviewed changes

fish-sammy reviewed Nov 12, 2024

View reviewed changes

cjcobb23 added 3 commits November 12, 2024 16:21

review

16f8aec

cleanup

e9a08da

rm comment

9c64ee9

fish-sammy reviewed Nov 12, 2024

View reviewed changes

milapsheth reviewed Nov 12, 2024

View reviewed changes

Merge branch 'main' into skip_invariant_p2p

7dd4581

cgorenflo closed this Dec 19, 2024

cgorenflo deleted the skip_invariant_p2p branch December 19, 2024 16:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

cjcobb23 commented Nov 12, 2024 •

edited

Loading

Uh oh!

codecov bot commented Nov 12, 2024 •

edited

Loading

Uh oh!

milapsheth Nov 12, 2024

Uh oh!

fish-sammy Nov 12, 2024 •

edited

Loading

Uh oh!

cjcobb23 Nov 12, 2024

Uh oh!

fish-sammy Nov 12, 2024 •

edited

Loading

Uh oh!

fish-sammy Nov 12, 2024

Uh oh!

fish-sammy Nov 12, 2024

Uh oh!

milapsheth Nov 12, 2024

Uh oh!

milapsheth Nov 12, 2024

Uh oh!

cjcobb23 Nov 13, 2024

Uh oh!

cgorenflo commented Dec 19, 2024

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

cjcobb23 commented Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

milapsheth Nov 12, 2024

Choose a reason for hiding this comment

Uh oh!

fish-sammy Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjcobb23 Nov 12, 2024

Choose a reason for hiding this comment

Uh oh!

fish-sammy Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fish-sammy Nov 12, 2024

Choose a reason for hiding this comment

Uh oh!

fish-sammy Nov 12, 2024

Choose a reason for hiding this comment

Uh oh!

milapsheth Nov 12, 2024

Choose a reason for hiding this comment

Uh oh!

milapsheth Nov 12, 2024

Choose a reason for hiding this comment

Uh oh!

cjcobb23 Nov 13, 2024

Choose a reason for hiding this comment

Uh oh!

cgorenflo commented Dec 19, 2024

Uh oh!

Uh oh!

cjcobb23 commented Nov 12, 2024 •

edited

Loading

codecov bot commented Nov 12, 2024 •

edited

Loading

fish-sammy Nov 12, 2024 •

edited

Loading

fish-sammy Nov 12, 2024 •

edited

Loading