Skip to content

Conversation

cptartur
Copy link
Member

@cptartur cptartur commented Oct 3, 2025

@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
Copy link
Contributor

Choose a reason for hiding this comment

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

There's lots of new code and logic here, which makes our branches even more overgrown 😅 . We should either move it to deploy function, or have separate wrapper (e.g. deploy_with_declare), which later calls deploy.

Copy link
Member Author

Choose a reason for hiding this comment

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

The way I wanted this to be implemented is that deploying with declaring is composed only using top level APIs, but they remain two entirely separate functions.

We could do a separate refactor of main and move out things that don't have to be there to separate modules. But I don't think hiding top level logic for functions is beneficial.

Copy link
Member Author

Choose a reason for hiding this comment

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

This logic is copied from declare and could technically be shared, but otherwise logic for declaring in deploy and normal declare are slightly different

            let manifest_path = assert_manifest_path_exists()?;
            let package_metadata = get_package_metadata(&manifest_path, &declare.package)?;
            let artifacts = build_and_load_artifacts(
                &package_metadata,
                &BuildConfig {
                    scarb_toml_path: manifest_path,
                    json: cli.json,
                    profile: cli.profile.unwrap_or("release".to_string()),
                },
                false,
                ui,
            )
            .expect("Failed to build contract");

Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't we have deploy_with_declare and deploy wrappers, which will share some common logic (most likely extracted into another function)?

Comment on lines 85 to 89
### Deploying and Declaring at the Same Time

You can deploy and declare a contract at the same time by passing `--contract-name` flag instead of `--class-hash`.
Under the hood, if the passed contract was never declared to starknet, it will run the [declare](../starknet/declare.md)
command first and then execute the contract deployment.
Copy link
Contributor

@franciszekjob franciszekjob Oct 3, 2025

Choose a reason for hiding this comment

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

nit: I think we should present this as deployment by contract name rather than deploying and declaring at the same time. The automatic declare is just a side effect, which allows for a simpler way to deploy without needing the class hash.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we should be hiding the fact that this makes 2 separate transactions. This is meant to be used as an convenience function, not magic to abstract how starknet works from users.

Copy link
Contributor

@franciszekjob franciszekjob Oct 6, 2025

Choose a reason for hiding this comment

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

I don't think we should be hiding the fact that this makes 2 separate transactions.

Definitely, that isn't my intention 😄

What I'm saying is that it's more suitable to promote this command as deployment by contract name. Also, title alone "Deploying and Declaring at the Same Time" may even suggest that this happens simultaneously (I know it's described below).

How about this small change?

### Deploying By Contract Name

You can deploy contract by passing `--contract-name` flag instead of `--class-hash`.
Under the hood, if the passed contract was never declared to starknet, it will run the [declare](../starknet/declare.md)
command first and then execute the contract deployment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good, will change it

@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
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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't we have deploy_with_declare and deploy wrappers, which will share some common logic (most likely extracted into another function)?

commit-id:82e786a7

# Conflicts:
#	crates/sncast/src/main.rs
@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
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.

2 participants