feat: allow multiple salt declarations in forc deploy (FuelLabs#4362)

## Description

Closes FuelLabs#4117 

Allow multiple salt declarations in `forc deploy`, with parsing and
validation involved. The gist of it is that it collects all the salt
inputs by the user as a `Vec<String>`, parses each input as a `Salt` and
store them in a `ContractSaltMap` matching contract->salt, and later
compares them against the contract dependencies to make sure that
duplicate salts do not exist.

This throws an error if:
1) multiple salts are present (if the same salt is declared but also
found within a contract dependency),
2) if a salt is invalid, 
3) if the salt input is invalid (there's a difference - the salt input
is the string `<CONTRACT>:<SALT>`, while the salt above is
`fuel_tx::Salt`, which has its own validation.
4) duplicate salts for the same contract were provided in the CLI.


Tests consist of dummy folders (not entire full package initialized by
forc) with only manifests to keep it the bare minimum required for these
tests.

## Checklist

- [x] I have linked to any relevant issues.
- [x] I have commented my code, particularly in hard-to-understand
areas.
- [ ] I have updated the documentation where relevant (API docs, the
reference, and the Sway book).
- [ ] I have added tests that prove my fix is effective or that my
feature works.
- [ ] I have added (or requested a maintainer to add) the necessary
`Breaking*` or `New Feature` labels where relevant.
- [ ] I have done my best to ensure that my PR adheres to [the Fuel Labs
Code Review
Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md).
- [ ] I have requested a review from the relevant team or maintainers.

---------

Co-authored-by: Mohammad Fawaz <[email protected]>

forc-pkg/src/manifest.rs

@@ -389,6 +389,11 @@ impl PackageManifestFile {
             Ok(self.dir().to_path_buf().join(constants::LOCK_FILE_NAME))
         }
     }
+
+    /// Returns an immutable reference to the project name that this manifest file describes.
+    pub fn project_name(&self) -> &str {
+        &self.project.name
+    }
 }
 
 impl PackageManifest {

forc-plugins/forc-client/src/cmd/deploy.rs

@@ -17,8 +17,17 @@ pub struct Command {
     pub gas: Gas,
     #[clap(flatten)]
     pub maturity: Maturity,
-    #[clap(flatten)]
-    pub salt: Salt,
+    /// Optional 256-bit hexadecimal literal(s) to redeploy contracts.
+    ///
+    /// For a single contract, use `--salt <SALT>`, eg.: forc deploy --salt 0x0000000000000000000000000000000000000000000000000000000000000001
+    ///
+    /// For a workspace with multiple contracts, use `--salt <CONTRACT_NAME>:<SALT>`
+    /// to specify a salt for each contract, eg.:
+    ///
+    /// forc deploy --salt contract_a:0x0000000000000000000000000000000000000000000000000000000000000001
+    /// --salt contract_b:0x0000000000000000000000000000000000000000000000000000000000000002
+    #[clap(long)]
+    pub salt: Option<Vec<String>>,
     /// Generate a random salt for the contract.
     /// Useful for testing or deploying examples to a shared network.
     #[clap(long)]

forc-plugins/forc-client/src/op/deploy.rs

@@ -9,20 +9,67 @@ use anyhow::{bail, Context, Result};
 use forc_pkg::{self as pkg, PackageManifestFile};
 use fuel_core_client::client::types::TransactionStatus;
 use fuel_core_client::client::FuelClient;
-use fuel_tx::{Output, TransactionBuilder};
+use fuel_tx::{Output, Salt, TransactionBuilder};
 use fuel_vm::prelude::*;
 use futures::FutureExt;
 use pkg::BuiltPackage;
-use std::path::PathBuf;
 use std::time::Duration;
+use std::{collections::BTreeMap, path::PathBuf};
 use sway_core::language::parsed::TreeType;
 use sway_core::BuildTarget;
 use tracing::info;
 
+#[derive(Debug)]
 pub struct DeployedContract {
     pub id: fuel_tx::ContractId,
 }
 
+type ContractSaltMap = BTreeMap<String, Salt>;
+
+/// Takes the contract member salt inputs passed via the --salt option, validates them against
+/// the manifests and returns a ContractSaltMap (BTreeMap of contract names to salts).
+fn validate_and_parse_salts<'a>(
+    salt_args: Vec<String>,
+    manifests: impl Iterator<Item = &'a PackageManifestFile>,
+) -> Result<ContractSaltMap> {
+    let mut contract_salt_map = BTreeMap::default();
+
+    // Parse all the salt arguments first, and exit if there are errors in this step.
+    for salt_arg in salt_args {
+        if let Some((given_contract_name, salt)) = salt_arg.split_once(':') {
+            let salt = salt
+                .parse::<Salt>()
+                .map_err(|e| anyhow::anyhow!(e))
+                .unwrap();
+
+            if let Some(old) = contract_salt_map.insert(given_contract_name.to_string(), salt) {
+                bail!("2 salts provided for contract '{given_contract_name}':\n  {old}\n  {salt}");
+            };
+        } else {
+            bail!("Invalid salt provided - salt must be in the form <CONTRACT_NAME>:<SALT> when deploying a workspace");
+        }
+    }
+
+    for manifest in manifests {
+        for (dep_name, contract_dep) in manifest.contract_deps() {
+            let dep_pkg_name = contract_dep.dependency.package().unwrap_or(dep_name);
+            if let Some(declared_salt) = contract_salt_map.get(dep_pkg_name) {
+                bail!(
+                    "Redeclaration of salt using the option '--salt' while a salt exists for contract '{}' \
+                    under the contract dependencies of the Forc.toml manifest for '{}'\n\
+                    Existing salt: '0x{}',\nYou declared: '0x{}'\n",
+                    dep_pkg_name,
+                    manifest.project_name(),
+                    contract_dep.salt,
+                    declared_salt,
+                    );
+            }
+        }
+    }
+
+    Ok(contract_salt_map)
+}
+
 /// Builds and deploys contract(s). If the given path corresponds to a workspace, all deployable members
 /// will be built and deployed.
 ///
@@ -40,30 +87,66 @@ pub async fn deploy(command: cmd::Deploy) -> Result<Vec<DeployedContract>> {
     let build_opts = build_opts_from_cmd(&command);
     let built_pkgs_with_manifest = built_pkgs_with_manifest(&curr_dir, build_opts)?;
 
-    // If a salt was specified but we have more than one member to build, there
-    // may be ambiguity in how the salt should be applied, especially if the
-    // workspace contains multiple contracts, and especially if one contract
-    // member is the dependency of another (in which case salt should be
-    // specified under `[contract- dependencies]`). Considering this, we have a
-    // simple check to ensure that we only accept salt when deploying a single
-    // package. In the future, we can consider relaxing this to allow for
-    // specifying a salt for workspace deployment, as long as there is only one
-    // root contract member in the package graph.
-    if command.salt.salt.is_some() && built_pkgs_with_manifest.len() > 1 {
-        bail!(
-            "A salt was specified when attempting to deploy a workspace with more than one member.
-              If you wish to deploy a contract member with salt, deploy the member individually.
-              If you wish to specify the salt for a contract dependency, \
-            please do so within the `[contract-dependencies]` table."
-        )
-    }
+    let contract_salt_map = if let Some(salt_input) = &command.salt {
+        // If we're building 1 package, we just parse the salt as a string, ie. 0x00...
+        // If we're building >1 package, we must parse the salt as a pair of strings, ie. contract_name:0x00...
+        if built_pkgs_with_manifest.len() > 1 {
+            let map = validate_and_parse_salts(
+                salt_input.clone(),
+                built_pkgs_with_manifest
+                    .iter()
+                    .map(|bpwm| bpwm.package_manifest_file()),
+            )?;
+
+            Some(map)
+        } else {
+            if salt_input.len() > 1 {
+                bail!("More than 1 salt was specified when deploying a single contract");
+            }
+
+            // OK to index into salt_input and built_pkgs_with_manifest here,
+            // since both are known to be len 1.
+
+            let salt = salt_input[0]
+                .parse::<Salt>()
+                .map_err(|e| anyhow::anyhow!(e))
+                .unwrap();
+            let mut contract_salt_map = ContractSaltMap::default();
+            contract_salt_map.insert(
+                built_pkgs_with_manifest[0]
+                    .package_manifest_file()
+                    .project_name()
+                    .to_string(),
+                salt,
+            );
+            Some(contract_salt_map)
+        }
+    } else {
+        None
+    };
 
-    for (member_manifest, built_pkg) in built_pkgs_with_manifest {
+    for built in built_pkgs_with_manifest {
+        let member_manifest = built.package_manifest_file();
         if member_manifest
             .check_program_type(vec![TreeType::Contract])
             .is_ok()
         {
-            let contract_id = deploy_pkg(&command, &member_manifest, &built_pkg).await?;
+            let salt = match (&contract_salt_map, command.random_salt) {
+                (Some(map), false) => {
+                    if let Some(salt) = map.get(member_manifest.project_name()) {
+                        *salt
+                    } else {
+                        Default::default()
+                    }
+                }
+                (None, true) => rand::random(),
+                (None, false) => Default::default(),
+                (Some(_), true) => {
+                    bail!("Both `--salt` and `--random-salt` were specified: must choose one")
+                }
+            };
+            let contract_id =
+                deploy_pkg(&command, member_manifest, built.built_package(), salt).await?;
             contract_ids.push(contract_id);
         }
     }
@@ -75,6 +158,7 @@ pub async fn deploy_pkg(
     command: &cmd::Deploy,
     manifest: &PackageManifestFile,
     compiled: &BuiltPackage,
+    salt: Salt,
 ) -> Result<DeployedContract> {
     let node_url = command
         .node_url
@@ -84,14 +168,7 @@ pub async fn deploy_pkg(
     let client = FuelClient::new(node_url)?;
 
     let bytecode = &compiled.bytecode.bytes;
-    let salt = match (command.salt.salt, command.random_salt) {
-        (Some(salt), false) => salt,
-        (None, true) => rand::random(),
-        (None, false) => Default::default(),
-        (Some(_), true) => {
-            bail!("Both `--salt` and `--random-salt` were specified: must choose one")
-        }
-    };
+
     let mut storage_slots = compiled.storage_slots.clone();
     storage_slots.sort();
 
@@ -179,3 +256,105 @@ fn build_opts_from_cmd(cmd: &cmd::Deploy) -> pkg::BuildOpts {
         member_filter: pkg::MemberFilter::only_contracts(),
     }
 }
+
+#[cfg(test)]
+mod test {
+    use super::*;
+
+    fn setup_manifest_files() -> BTreeMap<String, PackageManifestFile> {
+        let mut contract_to_manifest = BTreeMap::default();
+
+        let manifests_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR"))
+            .join("test")
+            .join("data")
+            .join("manifests");
+        for entry in manifests_dir.read_dir().unwrap() {
+            let manifest =
+                PackageManifestFile::from_file(entry.unwrap().path().join("Forc.toml")).unwrap();
+            contract_to_manifest.insert(manifest.project_name().to_string(), manifest);
+        }
+
+        contract_to_manifest
+    }
+
+    #[test]
+    fn test_parse_and_validate_salts_pass() {
+        let mut manifests = setup_manifest_files();
+        let mut expected = ContractSaltMap::new();
+        let mut salt_strs = vec![];
+
+        // Remove contracts with dependencies
+        manifests.remove("contract_with_dep_with_salt_conflict");
+        manifests.remove("contract_with_dep");
+
+        for (index, manifest) in manifests.values().enumerate() {
+            let salt = "0x0000000000000000000000000000000000000000000000000000000000000000";
+
+            let salt_str = format!("{}:{salt}", manifest.project_name());
+            salt_strs.push(salt_str.to_string());
+
+            expected.insert(
+                manifest.project_name().to_string(),
+                salt.parse::<Salt>().unwrap(),
+            );
+
+            let got = validate_and_parse_salts(salt_strs.clone(), manifests.values()).unwrap();
+            assert_eq!(got.len(), index + 1);
+            assert_eq!(got, expected);
+        }
+    }
+
+    #[test]
+    fn test_parse_and_validate_salts_duplicate_salt_input() {
+        let manifests = setup_manifest_files();
+        let first_name = manifests.first_key_value().unwrap().0;
+        let salt: Salt = "0x0000000000000000000000000000000000000000000000000000000000000000"
+            .parse()
+            .unwrap();
+        let salt_str = format!("{first_name}:{salt}");
+        let err_message =
+            format!("2 salts provided for contract '{first_name}':\n  {salt}\n  {salt}");
+
+        assert_eq!(
+            validate_and_parse_salts(vec![salt_str.clone(), salt_str], manifests.values())
+                .unwrap_err()
+                .to_string(),
+            err_message,
+        );
+    }
+
+    #[test]
+    fn test_parse_single_salt_multiple_manifests_malformed_input() {
+        let manifests = setup_manifest_files();
+        let salt_str =
+            "contract_a=0x0000000000000000000000000000000000000000000000000000000000000000";
+        let err_message =
+            "Invalid salt provided - salt must be in the form <CONTRACT_NAME>:<SALT> when deploying a workspace";
+
+        assert_eq!(
+            validate_and_parse_salts(vec![salt_str.to_string()], manifests.values())
+                .unwrap_err()
+                .to_string(),
+            err_message,
+        );
+    }
+
+    #[test]
+    fn test_parse_multiple_salts_conflict() {
+        let manifests = setup_manifest_files();
+        let salt_str =
+            "contract_with_dep:0x0000000000000000000000000000000000000000000000000000000000000001";
+        let err_message =
+            "Redeclaration of salt using the option '--salt' while a salt exists for contract 'contract_with_dep' \
+            under the contract dependencies of the Forc.toml manifest for 'contract_with_dep_with_salt_conflict'\n\
+            Existing salt: '0x0000000000000000000000000000000000000000000000000000000000000000',\n\
+            You declared: '0x0000000000000000000000000000000000000000000000000000000000000001'\n";
+
+        assert_eq!(
+            validate_and_parse_salts(vec![salt_str.to_string()], manifests.values())
+                .unwrap_err()
+                .to_string(),
+            err_message,
+        );
+    }
+}

forc-plugins/forc-client/src/op/run.rs

@@ -38,12 +38,13 @@ pub async fn run(command: cmd::Run) -> Result<Vec<RanScript>> {
     };
     let build_opts = build_opts_from_cmd(&command);
     let built_pkgs_with_manifest = built_pkgs_with_manifest(&curr_dir, build_opts)?;
-    for (member_manifest, built_pkg) in built_pkgs_with_manifest {
+    for built in built_pkgs_with_manifest {
+        let member_manifest = built.package_manifest_file();
         if member_manifest
             .check_program_type(vec![TreeType::Script])
             .is_ok()
         {
-            let pkg_receipts = run_pkg(&command, &member_manifest, &built_pkg).await?;
+            let pkg_receipts = run_pkg(&command, member_manifest, built.built_package()).await?;
             receipts.push(pkg_receipts);
         }
     }

forc-plugins/forc-client/src/util/pkg.rs

@@ -1,13 +1,28 @@
 use std::{collections::HashMap, path::Path, sync::Arc};
 
-use anyhow::Result;
+use anyhow::{bail, Result};
 use forc_pkg::{self as pkg, manifest::ManifestFile, BuildOpts, BuildPlan};
 use pkg::{build_with_options, BuiltPackage, PackageManifestFile};
 
+#[derive(Clone, Debug)]
+pub struct BuiltPackageWithManifest(Arc<BuiltPackage>, PackageManifestFile);
+
+impl BuiltPackageWithManifest {
+    /// Returns an immutable reference into the Arc<BuiltPackage>.
+    pub fn built_package(&self) -> &Arc<BuiltPackage> {
+        &self.0
+    }
+
+    /// Returns an immutable reference into the PackageManifestFile.
+    pub fn package_manifest_file(&self) -> &PackageManifestFile {
+        &self.1
+    }
+}
+
 pub(crate) fn built_pkgs_with_manifest(
     path: &Path,
     build_opts: BuildOpts,
-) -> Result<Vec<(PackageManifestFile, Arc<BuiltPackage>)>> {
+) -> Result<Vec<BuiltPackageWithManifest>> {
     let manifest_file = ManifestFile::from_dir(path)?;
     let mut member_manifests = manifest_file.member_manifests()?;
     let lock_path = manifest_file.lock_path()?;
@@ -24,16 +39,21 @@ pub(crate) fn built_pkgs_with_manifest(
     for member_index in build_plan.member_nodes() {
         let pkg = &graph[member_index];
         let pkg_name = &pkg.name;
-        // Check if the currrent member is built.
+        // Check if the current member is built.
         //
-        // For indivual members of the workspace, member nodes would be iterating
+        // For individual members of the workspace, member nodes would be iterating
         // over all the members but only the relevant member would be built.
         if let Some(built_pkg) = built_pkgs.remove(pkg) {
             let member_manifest = member_manifests
                 .remove(pkg_name)
                 .expect("Member manifest file is missing");
-            pkgs_with_manifest.push((member_manifest, built_pkg));
+            pkgs_with_manifest.push(BuiltPackageWithManifest(built_pkg, member_manifest));
         }
     }
+
+    if pkgs_with_manifest.is_empty() {
+        bail!("No built packages collected");
+    }
+
     Ok(pkgs_with_manifest)
 }