Add flag --all to cargo contract info #1319

smiasojed · 2023-09-07T10:05:08Z

Closes #797

Output for command cargo info -all is:

5DpyotvcUDkFxTtHzkwR7Rqe37bdjc6M5Rxeiy57Gv2RQzp8
5GsoMKLTgFEpzHQTi7c4ZKCLZC6VxtHZxwMydSi79uemABez

Flag combination --all --output-json outputs:

{
  "contracts": [
    "5DpyotvcUDkFxTtHzkwR7Rqe37bdjc6M5Rxeiy57Gv2RQzp8",
    "5GsoMKLTgFEpzHQTi7c4ZKCLZC6VxtHZxwMydSi79uemABez"
  ]
}

Flags combination --binary --all is forbidden
Current implementation does not support response paging

michalkucharczyk · 2023-09-07T14:55:07Z

crates/cargo-contract/src/cmd/info.rs

+                    // Binary flag applied
+                    (_, true) => {
+                        let wasm_code =
+                            fetch_wasm_code(*info_to_json.code_hash(), &client).await?;


nit: using ok_or may simplify code below by removing match.

crates/cargo-contract/src/cmd/info.rs

SkymanOne

Looks good. Just a few nitpicks and changes mainly to do with the coding style

SkymanOne · 2023-09-11T13:38:04Z

crates/cargo-contract/src/cmd/info.rs

+        // All flag applied
+        if self.all {
+            // 1000 is max allowed value
+            const COUNT: u32 = 1000;


Nit:

Suggested change

const COUNT: u32 = 1000;

const MAX_COUNT: u32 = 1000;

SkymanOne · 2023-09-11T13:42:16Z

crates/cargo-contract/src/cmd/info.rs

-        );
+    pub async fn run(&self) -> Result<(), ErrorVariant> {
+        let url = self.url.clone();
+        let client = OnlineClient::<DefaultConfig>::from_url(url).await?;


Suggested change

let client = OnlineClient::<DefaultConfig>::from_url(url).await?;

let client = OnlineClient::<DefaultConfig>::from_url(&self.url).await?;

SkymanOne · 2023-09-11T13:42:37Z

crates/cargo-contract/src/cmd/info.rs

-            self.contract
-        );
+    pub async fn run(&self) -> Result<(), ErrorVariant> {
+        let url = self.url.clone();


Suggested change

let url = self.url.clone();

SkymanOne · 2023-09-11T13:44:34Z

crates/extrinsics/src/lib.rs

@@ -378,6 +384,42 @@ pub async fn fetch_wasm_code(hash: CodeHash, client: &Client) -> Result<Option<V
    Ok(pristine_bytes)
 }

+/// Fetch all contracts addresses from the storage using the provided client and count of


Suggested change

/// Fetch all contracts addresses from the storage using the provided client and count of

/// Fetch all contract addresses from the storage using the provided client and count of

SkymanOne · 2023-09-11T13:46:31Z

crates/extrinsics/src/lib.rs

+pub async fn fetch_all_contracts(
+    client: &Client,
+    count: u32,
+    from: Option<&AccountId32>,


Suggested change

from: Option<&AccountId32>,

count_from: Option<&AccountId32>,

SkymanOne · 2023-09-11T13:46:49Z

crates/cargo-contract/src/cmd/info.rs

+        if self.all {
+            // 1000 is max allowed value
+            const COUNT: u32 = 1000;
+            let mut from = None;


Suggested change

let mut from = None;

let mut count_from = None;

SkymanOne · 2023-09-11T13:49:10Z

crates/extrinsics/src/lib.rs

+        .fetch_keys(key.as_ref(), count, start_key.as_deref())
+        .await?;
+
+    // StorageKey is a concatention of contract_info_of root key and


Suggested change

// StorageKey is a concatention of contract_info_of root key and

// StorageKey is a concatenation of contract_info_of root key and

SkymanOne · 2023-09-11T13:51:25Z

crates/extrinsics/src/lib.rs

+                e.0.get(key.len() + 8..)
+                    .ok_or(anyhow!("Unexpected storage key size"))?;
+            AccountId32::decode(&mut account)
+                .map_err(|err| anyhow!("AccountId deserialization error: {}", err))


I would extract it into a separate function for better readability

SkymanOne · 2023-09-11T13:55:51Z

crates/cargo-contract/src/cmd/mod.rs

+pub fn display_all_contracts(contracts: &[<DefaultConfig as Config>::AccountId]) {
+    contracts
+        .iter()
+        .for_each(|e: &<DefaultConfig as Config>::AccountId| {
+            name_value_println!("Contract", format!("{}", e), MAX_KEY_COL_WIDTH);
+        })
+}


I would rewrite it to display the result something like:

Found contracts: * <Address> * <Address> * <Address>

I think that in case when we have more than 1000 contracts, it will be hard to see this line Found contracts at the log beginning. I would opt to keep it as is. What do you think?

Then I would enumerate them at least. Useful when you want to pipe the output into another bash command:

Found contracts: 1. <Address> 2. <Address> ... 1000. <Address>

It is up to you if you want to keep Found contracts: or not.

Useful when you want to pipe the output into another bash command

In this case it is best then to have only the list of contract addresses so instead of

Contract 5DpyotvcUDkFxTtHzkwR7Rqe37bdjc6M5Rxeiy57Gv2RQzp8 Contract 5GsoMKLTgFEpzHQTi7c4ZKCLZC6VxtHZxwMydSi79uemABez

have

5DpyotvcUDkFxTtHzkwR7Rqe37bdjc6M5Rxeiy57Gv2RQzp8 5GsoMKLTgFEpzHQTi7c4ZKCLZC6VxtHZxwMydSi79uemABez

I like it this way, unix command compatible.

SkymanOne

LGTM! Thanks

smiasojed added 2 commits September 6, 2023 17:05

Add --all argument to info command

941c652

Add integration-tests for contract info --all, and code cleanup

74b43b7

smiasojed requested review from cmichi, ascjones and SkymanOne as code owners September 7, 2023 10:05

smiasojed added 4 commits September 7, 2023 12:07

Changelog with info --all support updated

efef0b1

Added error when out of range in slice for pallet map key

01e768c

Typo fixed in error message

0fdece8

Made the errors more readable

be59195

michalkucharczyk reviewed Sep 7, 2023

View reviewed changes

Code cleanup for info call

a4d93df

smiasojed requested a review from michalkucharczyk September 8, 2023 07:13

Simplified fetch_all_contracts function

a1d059b

ascjones reviewed Sep 11, 2023

View reviewed changes

Changed usize conversion and run method to async in info cmd

d59e201

SkymanOne suggested changes Sep 11, 2023

View reviewed changes

ascjones approved these changes Sep 11, 2023

View reviewed changes

smiasojed added 2 commits September 11, 2023 17:33

Code cleanup, typo in description fixed

01d8d5d

Variables renamed

3b056d5

SkymanOne approved these changes Sep 11, 2023

View reviewed changes

ascjones merged commit 5173489 into master Sep 12, 2023
11 checks passed

ascjones deleted the sm/contract-all branch September 12, 2023 11:55

smiasojed mentioned this pull request Nov 30, 2023

Release v4.0.0-rc #1415

Merged

smiasojed mentioned this pull request Mar 4, 2024

Release v4.0.0 #1518

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add flag --all to cargo contract info #1319

Add flag --all to cargo contract info #1319

smiasojed commented Sep 7, 2023 •

edited

michalkucharczyk Sep 7, 2023

smiasojed Sep 7, 2023

SkymanOne left a comment

SkymanOne Sep 11, 2023

smiasojed Sep 11, 2023

SkymanOne Sep 11, 2023

smiasojed Sep 11, 2023

SkymanOne Sep 11, 2023

smiasojed Sep 11, 2023

SkymanOne Sep 11, 2023

smiasojed Sep 11, 2023

SkymanOne Sep 11, 2023

smiasojed Sep 11, 2023

SkymanOne Sep 11, 2023

smiasojed Sep 11, 2023

SkymanOne Sep 11, 2023

smiasojed Sep 11, 2023

SkymanOne Sep 11, 2023

smiasojed Sep 11, 2023

SkymanOne Sep 11, 2023

smiasojed Sep 11, 2023 •

edited

SkymanOne Sep 11, 2023

ascjones Sep 11, 2023

SkymanOne Sep 11, 2023

smiasojed Sep 11, 2023

SkymanOne left a comment

	let client = OnlineClient::<DefaultConfig>::from_url(url).await?;
	let client = OnlineClient::<DefaultConfig>::from_url(&self.url).await?;

	/// Fetch all contracts addresses from the storage using the provided client and count of
	/// Fetch all contract addresses from the storage using the provided client and count of

	from: Option<&AccountId32>,
	count_from: Option<&AccountId32>,

	// StorageKey is a concatention of contract_info_of root key and
	// StorageKey is a concatenation of contract_info_of root key and

Add flag --all to cargo contract info #1319

Add flag --all to cargo contract info #1319

Conversation

smiasojed commented Sep 7, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SkymanOne left a 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

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

Choose a reason for hiding this comment

smiasojed Sep 11, 2023 • edited

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

SkymanOne left a comment

Choose a reason for hiding this comment

smiasojed commented Sep 7, 2023 •

edited

smiasojed Sep 11, 2023 •

edited