Global cast configuration #2689

kkawula · 2024-11-19T13:01:56Z

Closes #2221

Introduced changes

Added global config implementation
Update show-config command
Update snfoundry.toml docs

Checklist

crates/sncast/src/helpers/global_config.rs

crates/sncast/src/main.rs

crates/sncast/tests/e2e/main_tests.rs

docs/src/projects/configuration.md

franciszekjob · 2024-11-21T14:38:27Z

docs/src/projects/configuration.md

+#### Interaction between Local and Global profiles
+
+Interaction between global and local config is based on the overriding mechanism, the local config will be used to override 
+the global profile of the same name or in case it is not defined to override the default global profile.
+
+If `--profile` is not provided, the default profile from the local (if present) and global configuration will be used.
+


I wonder if we can add a little example, just to have a better illustration of how this mechanism works? Wdyt?

crates/sncast/Cargo.toml

crates/sncast/src/helpers/configuration.rs

crates/sncast/src/helpers/global_config.rs

cptartur · 2024-11-21T14:46:28Z

crates/sncast/src/helpers/global_config.rs

+    if !global_config_dir.exists() {
+        fs::create_dir_all(&global_config_dir)?;
+    }


I don't think we should be recursively creating this path. Let's only created a dir for starknet-foundry and if home/config dir is missing let's throw an error.

So use https://doc.rust-lang.org/beta/std/fs/fn.create_dir.html instead

Why not? I suppose that .config dir is intended for use by tools, and we should create it if is not present

Seems bit weird to me to create those.

But cc @THenry14 to verify

my understading is that create_dir_all corresponds to mkdir -p, which should be used in our case 👍

crates/sncast/src/main.rs

cptartur · 2024-11-21T15:04:00Z

docs/src/projects/configuration.md

+### Global configuration
+
+Since version 0.34.0 `sncast` will use the global configuration to combine configuration from local configs.
+Global configuration file is usual [`snfoundry.toml`](https://foundry-rs.github.io/starknet-foundry/appendix/snfoundry-toml.html), but placed in specific directory. 


We don't need this, these are the docs for latest version.

docs/src/projects/configuration.md

cptartur · 2024-11-21T15:11:40Z

docs/src/projects/configuration.md

+#### Interaction between Local and Global profiles
+
+Interaction between global and local config is based on the overriding mechanism, the local config will be used to override 
+the global profile of the same name or in case it is not defined to override the default global profile.
+
+If `--profile` is not provided, the default profile from the local (if present) and global configuration will be used.


This needs to be written in simpler terms, so it's obvious what overrides what and in which cases.

CHANGELOG.md

ddoktorski · 2024-11-21T15:29:20Z

crates/shared/src/consts.rs

@@ -1,3 +1,4 @@
 pub const EXPECTED_RPC_VERSION: &str = "0.7.0";
 pub const RPC_URL_VERSION: &str = "v0_7";
 pub const SNFORGE_TEST_FILTER: &str = "SNFORGE_TEST_FILTER";
+pub const FREE_RPC_PROVIDER_URL: &str = "https://free-rpc.nethermind.io/sepolia-juno/v0_7";


nit: Do we really want to hardcode a link to an external node in the template? This might require updates in the future. I suggest sticking with the previous devnet address http://127.0.0.1:5050.

I suppose that it will be convenient for the user to have ready sepolia URL defined.

Although it might be convenient for some users I still don't think we should include it in the template. If you add it here you are kind of responsible for it to work, which means extra effort to maintain it and potentially dealing with related support questions.

crates/sncast/src/helpers/global_config.rs

crates/sncast/src/main.rs

cptartur · 2024-11-22T11:05:32Z

crates/sncast/src/helpers/config.rs

+macro_rules! clone_field {
+    ($global_config:expr, $local_config:expr, $default_config:expr, $field:ident) => {
+        if $local_config.$field != $default_config.$field {
+            $local_config.$field.clone()
+        } else {
+            $global_config.$field.clone()
+        }
+    };
+}


I like having new macros, however in this case I think it just could be a function :ccc

Do we really want to introduce such ~~an ugly~~ function instead of this beautiful macro?

fn clone_filed<T: PartialEq + Clone>(global: &T, local: &T, default: &T) -> T { if *local != *default { local.clone() } else { global.clone() } }

crates/sncast/src/main.rs

cptartur · 2024-11-22T11:44:46Z

docs/src/projects/configuration.md

+**Glossary:**
+
+- **A:** Global configuration file containing the profiles `default` and `testnet`.
+- **B:** Local configuration file containing the profiles `default` and `mainnet`.
+
+In any directory in the file system, a user can run the `sncast` command using the `default` and `testnet` profiles, 
+because they are located in global config. 
+If no profiles are explicitly specified, the `default` profile from the global configuration file will be used.
+
+When running `sncast` from the `opus magnum` directory, there is a configuration file in the parent directory (B). 
+This setup allows for the use of the following profiles: `default`, `testnet`, and `mainnet`. If the `mainnet` profile is specified, 
+the configuration from the local file will be used to override the global `default` profile, as the `mainnet` profile does not exist in the global configuration.


It's a nice example but maybe show a shortened contents of A and B instead?

docs/src/projects/configuration.md

ddoktorski

In the commands account add and account import there is --add-profile flag. After introducing the global config it would be nice to explicitly mention that the created profile will be added to a local config if exists, or to change this logic. This can be done in this PR or a separate one, but it should be completed before the next release.

ddoktorski · 2024-11-22T12:17:53Z

CHANGELOG.md

@@ -24,6 +24,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

 - You can skip `--name` flag when using `account import` - a default name will be generated.
 - Addresses outputted when calling `sncast account create`, `sncast deploy` and `sncast declare` are now padded to 64 characters length and prefixed with `0x0`
+- Globally available configuration to store profiles to share between projects.
+- Missing fields in `show-config` command, `block_explorer` and `show_explorer_links`.
+- Default free sepolia testnet RPC URL for any `sncast` command. 


Even if we decide to add it, which I am strongly against, what is the point of mentioning this in the changelog?

I suppose that it is a significant change. It changes the expected behavior from the older version. Isn't it proper to mention here?

I see now that this also applies to the default CastConfig. I thought it was only for the template. That makes it even worse, but in such a case it should be added to the changelog.

Co-authored-by: Artur Michałek <52135326+cptartur@users.noreply.github.com>

kkawula added 3 commits November 19, 2024 14:01

Init

f55aaab

Update docs

fcc74f3

Lint

6717861

kkawula changed the title ~~Neque porro quisquam est qui dolorem~~ Global cast configuration Nov 19, 2024

kkawula added 8 commits November 19, 2024 14:22

Remove unnecessary test

ef6b1e8

Update change log

0b320f7

Update docs

6f1b1aa

Lint

336c3bf

Fmt

3e09212

Update show-config

9a88ff1

Update

4e6adea

Create rpc default in shared

65830c0

kkawula force-pushed the kkawula/2221-global-cast-configuration branch from 3d2e58e to 65830c0 Compare November 21, 2024 11:40

kkawula added 5 commits November 21, 2024 12:45

Merge branch 'master' into kkawula/2221-global-cast-configuration

d33e51c

Fix show-config tests

35fb640

Update building manifest

0f97dc7

Fix show-config tests

a212139

Update CHANGELOG.md

f2f4d0f

kkawula marked this pull request as ready for review November 21, 2024 13:35

kkawula requested review from cptartur, ddoktorski and franciszekjob as code owners November 21, 2024 13:35

franciszekjob requested changes Nov 21, 2024

View reviewed changes

cptartur requested changes Nov 21, 2024

View reviewed changes

ddoktorski reviewed Nov 21, 2024

View reviewed changes

kkawula added 5 commits November 21, 2024 17:00

Move dependency

1d4353b

Update CHANGELOG.md

6fe3ba1

pomidor

2b2a521

Add comma

1296c2c

Apply suggestions

a8a44e5

kkawula added 9 commits November 21, 2024 18:50

unwrap -> expect

fd4b8ce

Update default CastConfig

65b6c4d

Add macro

a0258a6

Fmt

cda64c9

Move a few functioins to different module

09d93d1

Fix show-config tests

da8116e

Lint

89fd8e7

Update docs

7b23dcd

Update docs

3db15c5

kkawula requested review from franciszekjob, cptartur and ddoktorski November 22, 2024 09:28

Update CHANGELOG.md

9c32742

cptartur requested changes Nov 22, 2024

View reviewed changes

ddoktorski reviewed Nov 22, 2024

View reviewed changes

kkawula and others added 3 commits November 22, 2024 18:08

Apply suggestions from code review

b6d7c00

Co-authored-by: Artur Michałek <52135326+cptartur@users.noreply.github.com>

Add link to default profiles

49d96f4

Rename update_cast_config -> update_config_with_cli

8fb474f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Global cast configuration #2689

Global cast configuration #2689

kkawula commented Nov 19, 2024 •

edited

Loading

franciszekjob Nov 21, 2024

cptartur Nov 21, 2024

kkawula Nov 21, 2024 •

edited

Loading

cptartur Nov 22, 2024

THenry14 Nov 22, 2024

cptartur Nov 21, 2024

cptartur Nov 21, 2024

ddoktorski Nov 21, 2024

kkawula Nov 21, 2024

ddoktorski Nov 22, 2024

cptartur Nov 22, 2024

kkawula Nov 22, 2024

cptartur Nov 22, 2024

ddoktorski left a comment

ddoktorski Nov 22, 2024

kkawula Nov 22, 2024

ddoktorski Nov 22, 2024

Global cast configuration #2689

Are you sure you want to change the base?

Global cast configuration #2689

Conversation

kkawula commented Nov 19, 2024 • edited Loading

Introduced changes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkawula Nov 21, 2024 • edited Loading

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

ddoktorski 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

kkawula commented Nov 19, 2024 •

edited

Loading

kkawula Nov 21, 2024 •

edited

Loading