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

Fix: stellar contract info * commands require network when network no… #1676

Merged
merged 12 commits into from
Nov 5, 2024

Conversation

Dhanraj30
Copy link
Contributor

…t required
#1653

What

Additional logic for checking network requirement
If WASM file is provided, don't require network arguments

Why

The current implementation of the stellar contract info commands requires network parameters (e.g., --rpc-url, --network-passphrase, --network) even when users are working with local files such as WebAssembly (.wasm) files. This requirement can lead to unnecessary errors and confusion for users who are only looking to interact with local resources.

This change introduces conditional checks in the command handling logic to ensure that network parameters are only enforced when they are actually needed

Known limitations

User Documentation: Existing documentation may need updates to clarify the new behavior of the commands concerning local .wasm file interactions. It is essential to ensure users are aware that network parameters are optional in these cases.

@willemneal
Copy link
Member

Currently your solution isn't correct. Clap parses commands and runs the corresponding "run" method. So in the case of this PR stellar network's run method is called and then it will call the relevant subcommand. In this PR that method is called e.g. network add. Then when it returns your logic is called. So it has no effect on the behavior of the subcommand nor on stellar contract info's subcommands. The solution to this PR is to edit the info's shared fetch_wasm, which currently resolves the network even if the wasm` arg is used which is a path to a local Wasm file. So the solution is to first check if the wasm arg is used and if so return early with the bytes. Otherwise you will need the network for the other two cases. So please remove the changes you have made here and edit this function.

pub async fn fetch_wasm(args: &Args) -> Result<Option<Vec<u8>>, Error> {
let network = &args.network.get(&args.locator)?;
let wasm = if let Some(path) = &args.wasm {
wasm::Args { wasm: path.clone() }.read()?
} else if let Some(wasm_hash) = &args.wasm_hash {
let hash = hex::decode(wasm_hash)
.map_err(|_| InvalidWasmHash(wasm_hash.clone()))?
.try_into()
.map_err(|_| InvalidWasmHash(wasm_hash.clone()))?;
let hash = xdr::Hash(hash);
let client = network.rpc_client()?;
client
.verify_network_passphrase(Some(&network.network_passphrase))
.await?;
get_remote_wasm_from_hash(&client, &hash).await?
} else if let Some(contract_id) = &args.contract_id {
let res = wasm::fetch_from_contract(contract_id, network, &args.locator).await;
if let Some(ContractIsStellarAsset) = res.as_ref().err() {
return Ok(None);
}
res?
} else {
unreachable!("One of contract location arguments must be passed");
};
Ok(Some(wasm))

Copy link
Member

@willemneal willemneal left a comment

Choose a reason for hiding this comment

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

LGTM just some small nits for removing extra new lines

cmd/soroban-cli/src/commands/global.rs Outdated Show resolved Hide resolved
cmd/soroban-cli/src/commands/global.rs Outdated Show resolved Hide resolved
cmd/soroban-cli/src/commands/network/mod.rs Outdated Show resolved Hide resolved
cmd/soroban-cli/src/commands/contract/info/shared.rs Outdated Show resolved Hide resolved
willemneal and others added 5 commits October 24, 2024 12:53
Co-authored-by: Willem Wyndham <willem@wyndham.tech>
Co-authored-by: Willem Wyndham <willem@wyndham.tech>
Co-authored-by: Willem Wyndham <willem@wyndham.tech>
Co-authored-by: Willem Wyndham <willem@wyndham.tech>
@Ifropc
Copy link
Contributor

Ifropc commented Oct 24, 2024

I'm pretty sure this won't work unless arguments are changed. From what I know Clap will parse arguments first, and we have network as one of the arguments, so I don't think this issue will be resolved with this PR

cargo run contract info interface --wasm ~/wasm/hello_world.wasm
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.17s
     Running `target/debug/soroban contract info interface --wasm /home/ifro/wasm/hello_world.wasm`
error: the following required arguments were not provided:
  --rpc-url <RPC_URL>
  --network-passphrase <NETWORK_PASSPHRASE>
  --network <NETWORK>

@willemneal
Copy link
Member

@Ifropc I fixed it so that clap doesn't require network options at all. This should fix this and make commands like keys generate only complain if the RPC is required, e.g. --fund.

@Ifropc
Copy link
Contributor

Ifropc commented Nov 1, 2024

@willemneal I think we can go with that approach, the only downside is that now arguments are no longer validated by clap. So we now will need to make sure we do the proper validation and give a good error message (e.g. current error message is not descriptive enough)

$ cargo run keys generate foo
   Compiling soroban-cli v21.5.0 (/home/ifro/projects/stellar-cli/cmd/soroban-cli)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 10.34s
     Running `target/debug/soroban keys generate foo`
⚠️ Behavior of `generate` will change in the future, and it will no longer fund by default. If you want to fund please provide `--fund` flag. If you don't need to fund your keys in the future, ignore this warning. It can be suppressed with -q flag.
❌ error: network arg or rpc url and network passphrase are required if using the network

@willemneal
Copy link
Member

@Ifropc FYI you can do cargo s which is the same as cargo run --quiet.

@Ifropc
Copy link
Contributor

Ifropc commented Nov 5, 2024

Fixed in #1698

@Ifropc Ifropc closed this Nov 5, 2024
@willemneal willemneal reopened this Nov 5, 2024
@willemneal
Copy link
Member

@Ifropc We still need this PR because it moves the network resolution after checking if there is a wasm arg.

@Ifropc
Copy link
Contributor

Ifropc commented Nov 5, 2024

Oh you are right sorry missed that

@Ifropc Ifropc enabled auto-merge (squash) November 5, 2024 21:43
@Ifropc Ifropc merged commit 3209130 into stellar:main Nov 5, 2024
50 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants