feat(minor-interchain-token-service): all chains with optional filter query message #754

maancham · 2025-02-05T20:11:46Z

Description

AXE-7369

Todos

Unit tests
Manual tests
Documentation
Connect epics/issues

Convention Checklist

Each contract should have a client mod for others to interact with it.
Derive macros
- EnsurePermissions: Contract permission control
- IntoContractError: Conversion from custom contract errors to axelar_wasm_std::error::ContractError
- IntoEvent: Event serialization
- migrate_from_version: Contract version management in the migrate function
The state mod and msg mod should use separate data structures so that internal state changes do not break the contract interface. Check out the interchain-token-service for reference.
- msg.rs should never use any type from the state.rs
- Shared types must be defined in a separate exported mod. If those types have already been defined somewhere else, then they should get re-exported in the exported mod

Steps to Test

Expected Behaviour

Notes

codecov · 2025-02-05T20:18:42Z

Codecov Report

Attention: Patch coverage is 97.56098% with 1 line in your changes missing coverage. Please review.

Project coverage is 93.66%. Comparing base (541aad9) to head (ed452c9).

Files with missing lines	Patch %	Lines
...cts/interchain-token-service/src/contract/query.rs	94.73%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #754   +/-   ##
=======================================
  Coverage   93.65%   93.66%           
=======================================
  Files         222      222           
  Lines       36332    36373   +41     
=======================================
+ Hits        34028    34068   +40     
- Misses       2304     2305    +1

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

cjcobb23 · 2025-02-05T20:12:39Z

contracts/interchain-token-service/src/msg.rs

@@ -85,6 +96,10 @@ pub enum QueryMsg {
    #[returns(HashMap<ChainNameRaw, Address>)]
    AllItsContracts,

+    /// Query all chain configs with optional frozen filter
+    #[returns(Vec<ChainConfigResponse>)]
+    ItsChains { filter: Option<ChainConfigFilter> },


do we want to paginate this now?

@cgorenflo Do we have an estimate on the rate of chain registration OR the current number of registered chains on ITS?

contracts/interchain-token-service/src/msg.rs

contracts/interchain-token-service/src/contract/query.rs

cgorenflo · 2025-02-06T22:10:07Z

contracts/interchain-token-service/src/contract/query.rs

+        Some(msg::ChainStatusFilter::Frozen) => config.frozen,
+        Some(msg::ChainStatusFilter::Active) => !config.frozen,


I would suggest to implement a matches function directly on the ChainStatusFilter

cgorenflo · 2025-02-06T22:11:43Z

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

@@ -137,6 +139,9 @@ pub fn query(deps: Deps, _: Env, msg: QueryMsg) -> Result<Binary, ContractError>
        QueryMsg::AllItsContracts => {
            query::all_its_contracts(deps).change_context(Error::QueryAllItsContracts)
        }
+        QueryMsg::ItsChains { filter } => {


this also needs pagination to avoid gas overflow

cgorenflo · 2025-02-06T22:12:46Z

contracts/interchain-token-service/src/contract/query.rs

@@ -34,6 +34,30 @@ pub fn all_its_contracts(deps: Deps) -> Result<Binary, Error> {
    to_json_binary(&contract_addresses).change_context(Error::JsonSerialization)
 }

+pub fn its_chains(deps: Deps, filter: Option<msg::ChainFilter>) -> Result<Binary, Error> {
+    let state_configs = state::load_chain_configs(deps.storage).change_context(Error::State)?;


the load_chain_configs should make use of pagination as well. Loading all configs from storage could lead to gas overflow later on

cgorenflo · 2025-02-06T22:17:42Z

contracts/interchain-token-service/src/contract/query.rs

+    let chain_configs = match filter {
+        Some(filter) if filter.frozen_status.is_some() => state_configs
+            .into_iter()
+            .filter(|config| matches_filter(config, filter.frozen_status.as_ref()))
+            .collect(),
+        _ => state_configs,


Suggested change

let chain_configs = match filter {

Some(filter) if filter.frozen_status.is_some() => state_configs

.into_iter()

.filter(|config| matches_filter(config, filter.frozen_status.as_ref()))

.collect(),

_ => state_configs,

let chain_configs = match filter {

Some(msg::ChainFilter {

frozen_status: Some(frozen_status),

}) => state_configs

.into_iter()

.filter(|config| frozen_status.matches(config))

.collect(),

_ => state_configs,

cgorenflo · 2025-02-06T22:18:58Z

contracts/interchain-token-service/src/state.rs

@@ -170,6 +170,27 @@ pub fn load_chain_config(
        .ok_or_else(|| report!(Error::ChainNotFound(chain.to_owned())))
 }

+pub fn load_chain_configs(storage: &dyn Storage) -> Result<Vec<msg::ChainConfigResponse>, Error> {
+    let configs = CHAIN_CONFIGS
+        .range(storage, None, None, Order::Ascending)


as mentioned before, this shouldn't be an unbounded range query

cgorenflo · 2025-02-06T22:21:26Z

contracts/interchain-token-service/src/state.rs

+        .map(|res| {
+            res.change_context(Error::Storage)
+                .map(|(chain, config)| msg::ChainConfigResponse {
+                    chain,
+                    its_edge_contract: config.its_address,
+                    truncation: msg::TruncationConfig {
+                        max_uint: config.truncation.max_uint,
+                        max_decimals_when_truncating: config
+                            .truncation
+                            .max_decimals_when_truncating,
+                    },
+                    frozen: config.frozen,
+                })
+        })
+        .collect::<Result<Vec<_>, _>>()?;


use map_ok(...).try_collect() instead

cgorenflo · 2025-02-06T22:24:53Z

contracts/interchain-token-service/src/state.rs

+                .map(|(chain, config)| msg::ChainConfigResponse {
+                    chain,
+                    its_edge_contract: config.its_address,
+                    truncation: msg::TruncationConfig {
+                        max_uint: config.truncation.max_uint,
+                        max_decimals_when_truncating: config
+                            .truncation
+                            .max_decimals_when_truncating,
+                    },
+                    frozen: config.frozen,


I would suggest to extract the mapping into it's own function, it's a separate concern to convert it into a msg type from loading it from storage. In fact, I think it's cleaner to return the actual chain configs here, and convert it in a separate step, at the call site

maancham added 4 commits February 5, 2025 12:18

feat: add ItsChains query message and utility

8ac6a2c

chore: add four unit tests

ab84a11

Merge branch 'main' into feat/its-chains

7a7a87c

fix: run nightly cargo fmt

4d4ac38

cjcobb23 reviewed Feb 5, 2025

View reviewed changes

maancham added 2 commits February 5, 2025 16:17

fix(lint): run with clippy 1.78 and remove vec!

9140c07

refactor: address PR comments

9985b78

maancham marked this pull request as ready for review February 6, 2025 16:00

maancham requested a review from a team as a code owner February 6, 2025 16:00

maancham added 3 commits February 6, 2025 12:55

feat: add test config setup to its unit tests

783c65a

fix: lint cargo clippy

fae8fc9

fix: lint issues with fmt

ed452c9

cgorenflo reviewed Feb 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(minor-interchain-token-service): all chains with optional filter query message #754

feat(minor-interchain-token-service): all chains with optional filter query message #754

maancham commented Feb 5, 2025 •

edited

Loading

codecov bot commented Feb 5, 2025 •

edited

Loading

cjcobb23 Feb 5, 2025

maancham Feb 5, 2025

cgorenflo Feb 6, 2025

cgorenflo Feb 6, 2025

cgorenflo Feb 6, 2025

cgorenflo Feb 6, 2025

cgorenflo Feb 6, 2025

cgorenflo Feb 6, 2025

cgorenflo Feb 6, 2025

		Some(msg::ChainStatusFilter::Frozen) => config.frozen,
		Some(msg::ChainStatusFilter::Active) => !config.frozen,

feat(minor-interchain-token-service): all chains with optional filter query message #754

Are you sure you want to change the base?

feat(minor-interchain-token-service): all chains with optional filter query message #754

Conversation

maancham commented Feb 5, 2025 • edited Loading

Description

Todos

Convention Checklist

Steps to Test

Expected Behaviour

Notes

codecov bot commented Feb 5, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maancham commented Feb 5, 2025 •

edited

Loading

codecov bot commented Feb 5, 2025 •

edited

Loading