Added Scarb workspaces for Cast #1093

Radinyn · 2023-11-10T11:56:26Z

Introduced changes

Checklist

THenry14

overall the logic looks good 👍

benchmarks/benchmarks copy.csv

crates/cast/src/helpers/build.rs

crates/cast/src/main.rs

crates/cast/tests/e2e/main_tests.rs

crates/cast/src/helpers/build.rs

Cargo.toml

MaksymilianDemitraszek

I generally don't understand --package filter from the perspective of cast. What purpose does it serve exactly?

MaksymilianDemitraszek · 2023-12-07T16:23:33Z

crates/cast/src/main.rs

+    which::which("scarb")
+        .context("Cannot find `scarb` binary in PATH. Make sure you have Scarb installed https://github.com/software-mansion/scarb")?;


@piotmag769 what was the reason we require it for all comands in forge?

MaksymilianDemitraszek · 2023-12-07T16:25:13Z

crates/cast/src/main.rs

+    pub package: Result<PackageMetadata>,
+}
+
+// Lazily get all the required package information from CLI


I don't understand the argument. What is the difference between calling different functions from different places instead of unpacking the errors in those places. It only adds complexity (we have to pass PackageData around) and is heavily not idiomatic. Please change that.

MaksymilianDemitraszek · 2023-12-07T16:27:29Z

crates/cast/src/main.rs

+    };
+
+    // Check if the path is actually valid
+    result.manifest_path = match path {


Please unpack path with ? in the part above.

MaksymilianDemitraszek · 2023-12-07T16:27:44Z

crates/cast/src/main.rs

+    // Check if the path is actually valid
+    result.manifest_path = match path {
+        Ok(path) => {
+            if path.exists() {


Please move this part to a separate function.

MaksymilianDemitraszek · 2023-12-07T16:29:13Z

crates/cast/src/helpers/build.rs

+use super::constants::LIB_CONTRACT_ARTIFACTS_NAME;
+
+#[allow(clippy::too_many_lines)]
+pub fn build(


It is good practice to have small functions doing only one thing. Current interface makes it not obvious that scarb writes to the memory. Declare shouldn't load scripts.

MaksymilianDemitraszek · 2023-12-07T16:46:51Z

crates/cast/src/main.rs

+        .context("Cannot find `scarb` binary in PATH. Make sure you have Scarb installed https://github.com/software-mansion/scarb")?;
+
+    let package_data = get_package_data(&cli);
+    let mut config = match &package_data.package {


Suggested change

let mut config = match &package_data.package {

let mut cast_config = match &package_data.package_metadata {

MaksymilianDemitraszek · 2023-12-07T16:52:00Z

crates/cast/src/main.rs

@@ -27,13 +33,16 @@ mod starknet_commands;
 #[allow(clippy::struct_excessive_bools)]
 struct Cli {
    /// Profile name in Scarb.toml config file
-    #[clap(short, long)]
+    #[clap(short = 'p', long)]


Not related change, please revert it

Is it? I think it is to override the -p option as a short option for --package (not sure how clap handles 2 flags with the same short form)

MaksymilianDemitraszek · 2023-12-07T16:52:03Z

crates/cast/src/main.rs

    profile: Option<String>,

-    /// Path to Scarb.toml that is to be used; overrides default behaviour of searching for scarb.toml in current or parent directories
+    /// Path to Scarb.toml that is to be used; overrides default behaviour of searching for Scarb.toml in current or parent directories


Not related change, please revert it

MaksymilianDemitraszek · 2023-12-07T16:54:07Z

docs/src/appendix/cast/common.md

+
+## `--package`


What is the purpose of this flag? How it behaves when I call a script, How it behaves when i call declare.

MaksymilianDemitraszek · 2023-12-07T17:02:29Z

crates/cast/tests/helpers/fixtures.rs

@@ -226,6 +226,23 @@ pub fn duplicate_directory_with_salt(src_path: String, to_be_salted: &str, salt:
    temp_dir
 }

+#[must_use]
+pub fn duplicate_directory(src_path: String) -> TempDir {


probably copy_directory_to_tempdir?

MaksymilianDemitraszek · 2023-12-08T11:06:36Z

Also we probably shouldn't take the first match, we should throw an error when it is ambigous

MaksymilianDemitraszek · 2023-12-08T11:16:01Z

We discussed it with @THenry14 , filter mechanism does not make much sense in the context of scripts.
Instead we should be able to pass module path to script instead of a name:
script package_name::path::to::script_module

If only script_module is passed and it is ambigous in the context of workspace we should print something like this:

package_namea::path::to::script_module
package_nameb::path::to::script_module
package_nameb::path2::to::script_module

Passing to::script_module also should be possible

piotmag769 · 2023-12-22T17:21:28Z

I generally don't understand --package filter from the perspective of cast. What purpose does it serve exactly?

@MaksymilianDemitraszek --package makes sense, you can choose an explicit package that config will be taken from etc.. I don't understand what --workspace does. It was decided that --workspace will be disabled, right? I don't see it explicitly disabled, looks like you can still pass it for a workspace with a single package (match_one won't fail).

Also what config from Scarb.toml takes priority when both --manifest-path and --package are both passed? Should it be even allowed? (currently it is from the CLI point of view)

piotmag769

Good bye 👋 👽 😢 🌹

piotmag769 · 2023-12-22T17:23:28Z

crates/sncast/src/helpers/build.rs

+            .join(&metadata.current_profile)
+            .join(sierra_filename);
+
+        let lib_artifacts = StarknetContractArtifacts {


Storing it together with StarknetContractArtifacts is kinda a hack, should be stored in a separate struct prob

maybe introduce a type alias?

piotmag769 · 2023-12-22T17:26:03Z

crates/sncast/src/helpers/scarb_utils.rs

@@ -276,23 +259,40 @@ pub fn get_package_tool_sncast(metadata: &scarb_metadata::Metadata) -> Result<&V
    Ok(tool_sncast)
 }

+pub fn get_first_package_from_metadata(metadata: &Metadata) -> Result<PackageMetadata> {


It terrifies me (I know this logic has been there before) xD Is it only used in tests? If so can we move it to the tests mod?

It's used in tests in multiple modules

## Introduced changes  - Opens up a discussion about sncast declare/script interfaces, first raised under this PR #1093 ## Checklist  - [X] Linked relevant issue - [X] Updated relevant documentation - [X] Added relevant tests - [X] Performed self-review of the code - [X] Added changes to `CHANGELOG.md`

Radinyn added 3 commits November 7, 2023 15:09

wip

a125310

wip

889fe6c

wip

52098b0

Radinyn self-assigned this Nov 10, 2023

wip

d553ffc

Radinyn mentioned this pull request Nov 14, 2023

Deployment scripts: add sncast_std with the call command #1067

Merged

5 tasks

Radinyn added 13 commits November 14, 2023 16:18

wip

1cdd735

Bump scarb-ui

c72332f

fmt

9555f6d

Using package filter

f8ca2b1

deps added

f67393c

wip

d26a1dd

Issues after scripts merge

419d351

merge

5cb6490

wip

832facf

wip

b001adc

only calldata tests fail

0a6c3a1

Tests passing

c650e72

Lint

213b583

Radinyn marked this pull request as ready for review November 29, 2023 16:08

Radinyn requested a review from a team as a code owner November 29, 2023 16:08

Radinyn requested review from DelevoXDG, abulenok and THenry14 and removed request for a team November 29, 2023 16:08

Merge branch 'master' into 449-cast-scarb-workspaces

534293a

DelevoXDG mentioned this pull request Nov 30, 2023

Pass current_dir to runner #1110

Closed

5 tasks

THenry14 reviewed Nov 30, 2023

View reviewed changes

benchmarks/benchmarks copy.csv Outdated Show resolved Hide resolved

crates/cast/src/helpers/build.rs Outdated Show resolved Hide resolved

crates/cast/src/main.rs Outdated Show resolved Hide resolved

crates/cast/tests/e2e/main_tests.rs Outdated Show resolved Hide resolved

THenry14 reviewed Nov 30, 2023

View reviewed changes

crates/cast/src/helpers/build.rs Outdated Show resolved Hide resolved

abulenok reviewed Dec 1, 2023

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

Radinyn added 11 commits December 6, 2023 19:02

merge with master

4dafd90

Fixed error introduced in other commits

5eba9b9

fix

f50ebf7

Fix error msgs

fc7c771

Fix forg e2e tests???

991960d

Fix naming

461f614

even more naming fixes

92b3568

much more naming changes

5e96d40

Tests pass

1d963d2

Fix contracts

d93b0f4

fix typo in tests

882ff62

Radinyn requested a review from THenry14 December 6, 2023 19:30

Radinyn and others added 4 commits December 6, 2023 21:45

Fixed forge e2e tests

f2f6198

renamed hello_workspaces

aa1053a

hopefully last rename

0a66c80

Merge branch 'master' into 449-cast-scarb-workspaces

4fa9fea

THenry14 requested review from MaksymilianDemitraszek and abulenok December 7, 2023 09:19

MaksymilianDemitraszek requested changes Dec 7, 2023

View reviewed changes

Radinyn added 2 commits December 22, 2023 02:54

address many things, merge

4de42ee

lint & fmt

d2a948c

piotmag769 reviewed Dec 22, 2023

View reviewed changes

THenry14 mentioned this pull request Jan 4, 2024

Add design doc for new declare/script interface #1485

Merged

5 tasks

This was referenced Jan 11, 2024

Use old approach to create accounts using sncast software-mansion/starknet.swift#130

Open

Remove sncast json workaround in DevnetClient software-mansion/starknet.swift#113

Open

THenry14 closed this Jan 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Scarb workspaces for Cast #1093

Added Scarb workspaces for Cast #1093

Radinyn commented Nov 10, 2023 •

edited by THenry14

Loading

THenry14 left a comment

MaksymilianDemitraszek left a comment

MaksymilianDemitraszek Dec 7, 2023

MaksymilianDemitraszek Dec 7, 2023

MaksymilianDemitraszek Dec 7, 2023

MaksymilianDemitraszek Dec 7, 2023

MaksymilianDemitraszek Dec 7, 2023

MaksymilianDemitraszek Dec 7, 2023

MaksymilianDemitraszek Dec 7, 2023

piotmag769 Dec 22, 2023

MaksymilianDemitraszek Dec 7, 2023

MaksymilianDemitraszek Dec 7, 2023

MaksymilianDemitraszek Dec 7, 2023

MaksymilianDemitraszek commented Dec 8, 2023

MaksymilianDemitraszek commented Dec 8, 2023 •

edited

Loading

piotmag769 commented Dec 22, 2023

piotmag769 left a comment

piotmag769 Dec 22, 2023

abulenok Dec 27, 2023

piotmag769 Dec 22, 2023

Radinyn Dec 22, 2023

		which::which("scarb")
		.context("Cannot find `scarb` binary in PATH. Make sure you have Scarb installed https://github.com/software-mansion/scarb")?;

	let mut config = match &package_data.package {
	let mut cast_config = match &package_data.package_metadata {

Added Scarb workspaces for Cast #1093

Added Scarb workspaces for Cast #1093

Conversation

Radinyn commented Nov 10, 2023 • edited by THenry14 Loading

Introduced changes

Checklist

THenry14 left a comment

Choose a reason for hiding this comment

MaksymilianDemitraszek 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

MaksymilianDemitraszek commented Dec 8, 2023

MaksymilianDemitraszek commented Dec 8, 2023 • edited Loading

piotmag769 commented Dec 22, 2023

piotmag769 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

Radinyn commented Nov 10, 2023 •

edited by THenry14

Loading

MaksymilianDemitraszek commented Dec 8, 2023 •

edited

Loading