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.

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

Query wasm contract state #NTRN-476 #96

Merged
merged 8 commits into from
Jun 30, 2023

Conversation

foxpy
Copy link
Contributor

@foxpy foxpy commented May 30, 2023

This PR adds wasm contract state query helper and example test on how to query wasm state using ICQ module.

How to review:

  • navigate to scripts/test_icq_wasm_juno_testnet/
  • read README.md
  • follow instructions and see whether everything works

[Assigned task]

@foxpy foxpy closed this May 30, 2023
@foxpy foxpy force-pushed the feat/NTRN-476_wasm_contract_state_icq branch from e9d4ae2 to 3841c59 Compare May 30, 2023 11:38
@foxpy foxpy reopened this May 30, 2023
@foxpy foxpy force-pushed the feat/NTRN-476_wasm_contract_state_icq branch 2 times, most recently from b3b537b to 45ef7ce Compare June 6, 2023 06:02
@foxpy foxpy changed the title wip #NTRN-476 Query wasm contract state #NTRN-476 Jun 6, 2023
@foxpy foxpy marked this pull request as ready for review June 6, 2023 06:05
@foxpy foxpy marked this pull request as draft June 6, 2023 06:14
@foxpy foxpy force-pushed the feat/NTRN-476_wasm_contract_state_icq branch 2 times, most recently from ced6feb to 8f4e96d Compare June 6, 2023 11:48
@foxpy foxpy marked this pull request as ready for review June 6, 2023 12:05
@foxpy foxpy force-pushed the feat/NTRN-476_wasm_contract_state_icq branch from 8f4e96d to f4e855d Compare June 6, 2023 12:07
Comment on lines +211 to +214
let mut storage_key = vec![0u8, 7u8];

storage_key.extend_from_slice("balance".as_bytes());
storage_key.extend_from_slice(account_address.as_bytes());
Copy link
Collaborator

Choose a reason for hiding this comment

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

This stuff definitely should be in the SDK helpers not here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, it shouldn't be there, because these values are contract-dependent. I wish I could make a cw20 balance helper, but it is impossible to do.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think Mike means that it's not only new_register_wasm_contract_store_query_msg should be in the helpers package as the most generic helper for interacting with contract's storage, but a more specific helper to register cw20 balance query as well (e.g. new_register_cw20_balance_query_msg analogically to new_register_delegator_delegations_query_msg). the helper can have contract and holder addresses as parameters and assemble the needed key inside with these vec![0u8, 7u8] and balance reused from constants defined here: https://github.com/neutron-org/neutron-sdk/blob/main/packages/neutron-sdk/src/interchain_queries/v045/types.rs

Copy link
Contributor

Choose a reason for hiding this comment

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

but now I wonder if we need such a helper — to query a cw20 balance

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, I like it how it is now, a general new_register_wasm_contract_store_query_msg is what we need here whereas cw20 balance stuff is a too specific case to be in the sdk helpers package

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 could implement some kind of storage key factory like this:

let key: Vec<u8> =
  StorageKeyFactory::new()
  .push_bytes([0, 7])
  .push_string("balance")
  .push_string(account_address)
  .into_bytes();

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 am very hesitant creating even some kind of "general helper which works for canonical cw20-base contract" because users will definitely try to use it and it won't work in some cases.

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 could implement some kind of storage key factory like this:

To be honest it doesn't make anything easier to use, I think I should abandon this idea.

contracts/neutron_interchain_queries/src/contract.rs Outdated Show resolved Hide resolved
@foxpy foxpy requested a review from pr0n00gler June 15, 2023 15:53
@foxpy
Copy link
Contributor Author

foxpy commented Jun 15, 2023

I have no idea what's wrong about this pesky clippy workflow, everything works on my machine locally 🤷

Comment on lines +211 to +214
let mut storage_key = vec![0u8, 7u8];

storage_key.extend_from_slice("balance".as_bytes());
storage_key.extend_from_slice(account_address.as_bytes());
Copy link
Contributor

Choose a reason for hiding this comment

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

I think Mike means that it's not only new_register_wasm_contract_store_query_msg should be in the helpers package as the most generic helper for interacting with contract's storage, but a more specific helper to register cw20 balance query as well (e.g. new_register_cw20_balance_query_msg analogically to new_register_delegator_delegations_query_msg). the helper can have contract and holder addresses as parameters and assemble the needed key inside with these vec![0u8, 7u8] and balance reused from constants defined here: https://github.com/neutron-org/neutron-sdk/blob/main/packages/neutron-sdk/src/interchain_queries/v045/types.rs

@@ -241,6 +283,19 @@ fn query_recipient_txs(deps: Deps<NeutronQuery>, recipient: String) -> NeutronRe
Ok(to_binary(&GetRecipientTxsResponse { transfers: txs })?)
}

pub fn query_cw20_balance(
Copy link
Contributor

Choose a reason for hiding this comment

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

if we create a new_register_cw20_balance_query_msg in the helpers package, we should move this one to the helpers package as well

GetRegisteredQuery { query_id: u64 },
GetRecipientTxs { recipient: String },
}

#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq, JsonSchema)]
#[serde(rename_all = "snake_case")]
pub struct Cw20BalanceResponse {
Copy link
Contributor

Choose a reason for hiding this comment

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

if we create a new_register_cw20_balance_query_msg in the helpers package, we should move this one to the helpers package as well

@@ -40,7 +40,7 @@ pub fn query_kv_result<T: KVReconstruct>(
}

/// Queries interchain query result (raw KV storage values or transactions) from Interchain Queries Module
fn get_interchain_query_result(
pub fn get_interchain_query_result(
Copy link
Contributor

Choose a reason for hiding this comment

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

why you decided to make it pub? if we want to expose it, we should make it more obvious how it's different from the query_kv_result above and highlight use cases for both funcs. I mean in this case we need to give it a better naming which will reflect that it returns a raw kv storage value for example and elaborate description

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I agree, I think I will add "raw" to the name of this function and cover it with some decent documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

cw20_contract_address: String,
account_address: String,
) -> NeutronResult<Response<NeutronMsg>> {
// cw_storage_plus uses this prefix for maps
Copy link
Collaborator

Choose a reason for hiding this comment

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

There must be an instruction how did you get this prefix, it's not obvious

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bold of you to assume I remember how I did that.

Okay, I will write some extensive docs on how to examine contract storage paths.

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 have added some docs, please check them out

pr0n00gler
pr0n00gler previously approved these changes Jun 19, 2023
sotnikov-s
sotnikov-s previously approved these changes Jun 20, 2023
@MrTalecky
Copy link

@foxpy hi!
A few additions to the readme file:

  1. there is no make build command
  2. It's not clear what directory to run the scripts from

Perhaps the instructions should be added for users with M1 processors, which will have problems with the build.
You just need to do three things:

  1. Remove the file cargo.lock
  2. Run rm -rf target/
  3. Re-run make build

But it's up to you)

@pr0n00gler pr0n00gler merged commit 141d4b7 into main Jun 30, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants