Skip to content

Allow declaring with deployment #3783

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: spr/master/94945a36
Choose a base branch
from
Open

Allow declaring with deployment #3783

wants to merge 1 commit into from
+524 −54

Conversation

cptartur
Copy link
Member

@cptartur cptartur commented Oct 3, 2025 •
edited
Loading

This was referenced Oct 3, 2025
Open
Open
Open
Open
@cptartur cptartur marked this pull request as ready for review October 3, 2025 10:54
@cptartur cptartur requested a review from a team as a code owner October 3, 2025 10:54
@cptartur cptartur requested review from ddoktorski and franciszekjob and removed request for a team October 3, 2025 10:54
@cptartur cptartur force-pushed the spr/master/82e786a7 branch 2 times, most recently from ac92c0d to 48c85d9 Compare October 3, 2025 14:23
@cptartur cptartur force-pushed the spr/master/94945a36 branch from 899b363 to a9c96f6 Compare October 3, 2025 14:23
franciszekjob
franciszekjob reviewed Oct 3, 2025
View reviewed changes
@cptartur cptartur force-pushed the spr/master/82e786a7 branch from 48c85d9 to 859e21b Compare October 6, 2025 11:43
@cptartur cptartur force-pushed the spr/master/94945a36 branch from a9c96f6 to 41b8753 Compare October 6, 2025 11:43
@cptartur cptartur requested a review from franciszekjob October 6, 2025 11:46
@cptartur cptartur force-pushed the spr/master/94945a36 branch from 41b8753 to 39b967c Compare October 6, 2025 13:14
@cptartur cptartur force-pushed the spr/master/82e786a7 branch 2 times, most recently from 28cef8c to b764071 Compare October 6, 2025 13:29
franciszekjob
franciszekjob reviewed Oct 6, 2025
View reviewed changes
Copy link
Contributor

@franciszekjob franciszekjob left a comment

Choose a reason for hiding this comment

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

Wdyt about mentioning this in sncast 101?

@cptartur cptartur force-pushed the spr/master/82e786a7 branch from b764071 to 96a6140 Compare October 6, 2025 14:47
@cptartur cptartur force-pushed the spr/master/94945a36 branch from 39b967c to 0b18bd5 Compare October 6, 2025 14:47
@cptartur cptartur force-pushed the spr/master/82e786a7 branch from 96a6140 to 24199f7 Compare October 7, 2025 12:52
@cptartur cptartur force-pushed the spr/master/94945a36 branch from 0b18bd5 to 04e89a1 Compare October 7, 2025 12:52
@cptartur cptartur requested a review from franciszekjob October 7, 2025 12:52
@cptartur
Allow declaring with deployment
aaa6e8d 
commit-id:82e786a7

# Conflicts:
#	crates/sncast/src/main.rs
@cptartur cptartur force-pushed the spr/master/94945a36 branch from 04e89a1 to 5ea3fa5 Compare October 7, 2025 14:39
@cptartur cptartur force-pushed the spr/master/82e786a7 branch from 24199f7 to aaa6e8d Compare October 7, 2025 14:39
@cptartur cptartur linked an issue Oct 7, 2025 that may be closed by this pull request
Implement declare-deploy command in cast #3606
Open
franciszekjob
franciszekjob approved these changes Oct 7, 2025
View reviewed changes
docs/src/starknet/deploy.md
Comment on lines +122 to +126
To see deployment details, visit:
contract: [..]
class: [..]
declare transaction: [..]
deploy transaction: [..]
Copy link
Contributor

@franciszekjob franciszekjob Oct 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe I'm a bit too picky, but after deployment details we have link to declare transaction. Maybe it can be moved above/separated?

crates/sncast/src/response/deploy.rs
Comment on lines +17 to +18
Deploy(StandardDeployResponse),
DeployWithDeclare(DeployResponseWithDeclare),
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
Deploy(StandardDeployResponse),
DeployWithDeclare(DeployResponseWithDeclare),
Standard(StandardDeployResponse),
WithDeclare(DeployResponseWithDeclare),

crates/sncast/src/main.rs
}
}
} else {
unreachable!("One of class_hash or contract_name must be provided");
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
unreachable!("One of class_hash or contract_name must be provided");
unreachable!("One of `--class-hash` or `--contract-name` must be provided");

crates/sncast/src/starknet_commands/deploy.rs
Comment on lines +20 to +31
#[derive(Args, Debug, Clone)]
#[group(required = true, multiple = false)]
pub struct ContractIdentifier {
/// Class hash of contract to deploy
#[arg(short = 'g', long)]
pub class_hash: Option<Felt>,

/// Contract name
#[arg(long, conflicts_with = "class_hash")]
pub contract_name: Option<String>,
}

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Usage of id and group seems suitable here:

Suggested change
#[derive(Args, Debug, Clone)]
#[group(required = true, multiple = false)]
pub struct ContractIdentifier {
/// Class hash of contract to deploy
#[arg(short = 'g', long)]
pub class_hash: Option<Felt>,
/// Contract name
#[arg(long, conflicts_with = "class_hash")]
pub contract_name: Option<String>,
}
#[derive(Args, Debug, Clone)]
#[group(id = "contract_identifier", required = true, multiple = false)]
pub struct ContractIdentifier {
/// Class hash of contract to deploy
#[arg(short = 'g', long, group = "contract_identifier")]
pub class_hash: Option<Felt>,
/// Contract name
#[arg(long, group = "contract_identifier")]
pub contract_name: Option<String>,
}

ddoktorski
ddoktorski reviewed Oct 7, 2025
View reviewed changes
crates/sncast/src/main.rs
(class_hash, None)
} else if let Some(contract_name) = identifier.contract_name {
let manifest_path = assert_manifest_path_exists()?;
let package_metadata = get_package_metadata(&manifest_path, &None)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this feature work for workspaces?

crates/sncast/src/main.rs
WaitForTx {
wait: true,
wait_params: wait_config.wait_params,
show_ui_outputs: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

What if someone passes the --wait flag?

crates/sncast/tests/e2e/deploy.rs
}

#[tokio::test]
async fn test_deploy_with_declare_nonexistent_contract() {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the rationale behind this test given that its name refers to nonexistent_contract, but the asserted error is Invalid transaction nonce?

docs/src/starknet/deploy.md
> The contract might wait for a few seconds before executing the deployment.
> 📝 **Note**
> If fee arguments are provided to the method, they will be shared between declare and deploy transactions.
Copy link
Contributor

Choose a reason for hiding this comment

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

It might suggest that the provided fee will will try to cover both transactions, which is not the case. I think it should be rephrased to clarify that the same fee arguments are used separately for each transaction

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ddoktorski ddoktorski ddoktorski left review comments

@franciszekjob franciszekjob franciszekjob approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement declare-deploy command in cast
3 participants
@cptartur @ddoktorski @franciszekjob