-
Notifications
You must be signed in to change notification settings - Fork 25
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(minor-interchain-token-service): skip invariant checks for p2p #687
base: main
Are you sure you want to change the base?
Conversation
* 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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #687 +/- ##
==========================================
+ Coverage 93.60% 93.62% +0.02%
==========================================
Files 238 238
Lines 35570 35644 +74
==========================================
+ Hits 33294 33372 +78
+ Misses 2276 2272 -4 ☔ View full report in Codecov by Sentry. |
interceptors::subtract_supply_amount(storage, &source_chain, &transfer)?; | ||
} | ||
|
||
// we only possibly scale if both source chain and destination chain are amplifier chains |
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.
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
// 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)?; | ||
} |
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.
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)?;
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 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
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.
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.
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 |
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.
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), | |
} |
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.
Add a comment saying that the default is from the original chain.
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 |
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.
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) |
chain: &ChainSpecifier, | ||
token_id: TokenId, | ||
) -> Result<TokenInstance, Error> { | ||
let res = state::may_load_token_instance(storage, chain.name.clone(), token_id) |
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.
Querying here everytime for now will be much cleaner, the additional gas cost isn't a significant concern for now
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.
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
https://axelarnetwork.atlassian.net/browse/AXE-6434