fix(cli): Fix and clean client modes and fetch presets #279

antiyro · 2024-09-23T09:06:18Z

No description provided.

cchudant

list of stuff from #255 i'd like reverted/changed:

revert the set_workdir addition in the tests
update the example config file
revert the built in configs to functions like

    pub fn starknet_sepolia() -> Self {
        Self {
            chain_name: "Starknet Sepolia".into(),
            chain_id: ChainId::Sepolia,
            eth_core_contract_address: eth_core_contract_address::SEPOLIA_TESTNET.parse().expect("parsing a constant"),
            ..Self::starknet_mainnet()
        }
    }

and they do not need to return results
The preset can just use those functions instead of reading any file

remove any fs read except in tests, and when the chain config file is supplied as argument
please add this comment at the top of chain_config.rs

// Note: We are NOT using fs read for constants, as they NEED to be included in the resulting
// binary. Otherwise, using the madara binary without cloning the repo WILL crash, and that's very very bad.
// The binary needs to be self contained! We need to be able to ship madara as a single binary, without
// the user needing to clone the repo.
// Only use `fs` for constants when writing tests.

change the chain config override system (see my comment below)
Duration parsing needs to accept 2s, 200ms -- stings like that, and should not accept naked numbers. Change all the cli that currently accept durations to that format too.
I think we should not use test config for devnet, but a config named devnet. The test preset can be removed as it's only used in tests using ChainConfig::test_config
there are a few except and error handling stuff that could be better handled.

cchudant · 2024-09-23T12:52:46Z

crates/primitives/chain_config/src/chain_config.rs

+                self.get_preset_name()
+            );
+            let remote_url = match self {
+                ChainPreset::Mainnet => "https://raw.githubusercontent.com/madara-alliance/madara/main/crates/primitives/chain_config/presets/mainnet.yaml",


id prefer if we used include bytes here please
or, i think it's just better to revert all of it - remake the functions as they were before

pub fn starknet_sepolia() -> Self { Self { chain_name: "Starknet Sepolia".into(), chain_id: ChainId::Sepolia, eth_core_contract_address: eth_core_contract_address::SEPOLIA_TESTNET.parse().expect("parsing a constant"), ..Self::starknet_mainnet() } }

like that
it's much better i think, we really dont need to parse a file here

Reverted the read file stuff!

cchudant · 2024-09-23T12:53:49Z

crates/node/src/cli/mod.rs

            None => {
-                ChainConfig::from_yaml(&self.chain_config_path.clone().expect("Failed to retrieve chain config path"))?
+                let path = self.chain_config_path.clone().ok_or_else(|| {


can we just support --devnet without specifying any kind of preset please

Why would you want that, we would like to have custom devnets also (the kkt use case were they need to custom steps is already a use case for that)

well, i think --devnet without --chain-config implies --preset devnet

--devnet alone is now accepted - preset will be devnet by default & it's still possible to specify another config

cchudant · 2024-09-23T12:55:22Z

crates/node/src/cli/mod.rs

-        }
+        // Override stuff if flag is set
+        let chain_config =
+            if self.chain_config_override { self.chain_params.override_cfg(chain_config) } else { chain_config };


id prefer if you remade the chain config override system from scratch and follow how i described it in #240

which means, no new cli struct or field by field override by hand

As said in DMs, I've tried to implement a different override mechanism but it's far from perfect at the moment because ChainConfig doesn't implement Serialize (since our flow is 1. generate ChainConfig 2. override if possible , we need to convert the pre-built ChainConfig to Value).

Instead of going down the rabbit hole of implementing wrapper structs around Blockifiers one & implementing Serialize myself, I've created a wrapper struct OverridableChainConfig that represents the fields we're able to override.

I'm not really fan of this tbh but it works at least. Any thoughts? Implementing Serialize would appear the cleanest here probably but didn't try yet - can do if you judge it better

cchudant · 2024-09-23T12:55:54Z

crates/node/src/main.rs

+                Arc::new(ForwardToProvider::new(SequencerGatewayProvider::new(
+                    run_cmd
+                        .network
+                        .expect(


use context not expect here please

cchudant · 2024-09-23T13:06:48Z

i think preset can be moved to node cli args and not inside the primitive crate altogether -- to a clap enum. It would allow clap to show a proper error message when the preset is unknown, with a list of the possible values. Much better UX

cchudant · 2024-09-23T13:11:43Z

also, i dont know what the default impl on ChainConfig means, try to remove it and see if it was useful
(i just dont know why it's there it feels weird)

antiyro · 2024-09-25T16:15:39Z

This PR will be handled by @akhercha from now on

akhercha · 2024-09-27T11:17:30Z

Everything's done, just not sure about the ChainConfig overrides as I said above + this comment:

update the example config file

Wdym? I updated the comments inside but happy to do more. @cchudant

antiyro · 2024-09-27T12:52:20Z

configs/presets/devnet.yaml

@@ -9,8 +9,8 @@ versioned_constants:
  "0.13.2": "crates/primitives/chain_config/resources/versioned_constants_13_2.json"
 eth_core_contract_address: "0xE2Bb56ee936fd6433DC0F6e7e3b8365C906AA057"
 latest_protocol_version: "0.13.2"
-block_time: 360
-pending_block_update_time: 2
+block_time: "6min"


why 6mins block time over chain configs, we have around 30sec avg on mainnet

Not sure, it was like this before - but updated to 30s, u're right!

antiyro · 2024-09-27T12:52:44Z

configs/presets/devnet.yaml

@@ -30,5 +30,5 @@ bouncer_config:
    message_segment_length: 18446744073709551615
    n_events: 18446744073709551615
    state_diff_size: 131072
-sequencer_address: "0x123"
+sequencer_address: "0x211b748338b39fe8fa353819d457681aa50ac598a3db84cacdd6ece0a17e1f3"


what is this address exactly?

Woops, it was meant to be used for the test config. Removed!

antiyro

Tested locally and lgtm

…nto fix/cli

antiyro · 2024-09-27T13:50:55Z

configs/presets/devnet.yaml

@@ -30,5 +30,5 @@ bouncer_config:
    message_segment_length: 18446744073709551615
    n_events: 18446744073709551615
    state_diff_size: 131072
-sequencer_address: "0x123"
+sequencer_address: "0x0"


Sequencer address cannot be zero for block production. We should use another one for the devnet config

akhercha

LGTM

changed authority to sequencer mode

dc30d18

antiyro self-assigned this Sep 23, 2024

antiyro added bug Something isn't working enhancement New feature or request labels Sep 23, 2024

antiyro added 5 commits September 23, 2024 12:02

adding cli rules for full

26d2b50

improved error management

31696ba

testing missed cases

7c6c7d1

added support for remote fetch of configfile

31ec92c

fixing ci

a1bbc71

antiyro changed the title ~~fix(cli): Fix and clean client modes + fix db columns for metrics~~ fix(cli): Fix and clean client modes and fetch presets Sep 23, 2024

antiyro marked this pull request as ready for review September 23, 2024 12:31

cleaned for ci

bf28872

cchudant requested changes Sep 23, 2024

View reviewed changes

EvolveArt mentioned this pull request Sep 23, 2024

feat(test) block production integration tests #249

Closed

antiyro assigned akhercha Sep 25, 2024

akhercha added 12 commits September 26, 2024 22:50

fix(cli): ChainConfig fixes - new override behavior & simpler

de4572a

fix(cli): Sync with main

507ca3d

fix(cli): Fixed path in tests after removal of set_workdir

a3ae3fa

fix(cli): Docstring

cf757b1

fix(cli): Re-added test config

2ecb5f2

fix(cli): Renamed stuff

b129599

fix(cli): Moved ChainPreset to CLI

d4cac2a

fix(cli): Context instead of expect

2948e87

fix(cli): Human readable durations

d25ee54

Merge branch 'main' into fix/cli

45189f9

fix(cli): Parse duration tests

b722f96

fix(cli): CLI durations

6c1556d

akhercha added 4 commits September 27, 2024 13:36

fix(cli): Non mandatory config for --devnet

40df5c4

fix(cli): Checks during override

c343e17

fix(cli): Override example

01c1d81

fix(cli): better error

74dd1ed

antiyro commented Sep 27, 2024

View reviewed changes

akhercha and others added 3 commits September 27, 2024 15:32

fix(cli): Comments stuff

09802e0

reovered state root on full sync

11034e6

Merge branch 'fix/cli' of https://github.com/madara-alliance/madara i…

9d5ac82

…nto fix/cli

antiyro commented Sep 27, 2024

View reviewed changes

akhercha added 5 commits September 27, 2024 15:54

fix(cli): Fixed override

75b911e

fix(cli): removed test from CLI

05293fd

fix(cli): Sequencer default address for devnet

7ac59b4

fix(cli): Fixed path stuff

4ad2c3b

fix(cli): Fixed test

cfe3b3f

akhercha approved these changes Sep 30, 2024

View reviewed changes

antiyro merged commit e2c92eb into main Sep 30, 2024
6 of 7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cli): Fix and clean client modes and fetch presets #279

fix(cli): Fix and clean client modes and fetch presets #279

antiyro commented Sep 23, 2024

cchudant left a comment •

edited

Loading

cchudant Sep 23, 2024

akhercha Sep 27, 2024

cchudant Sep 23, 2024

antiyro Sep 23, 2024

cchudant Sep 23, 2024

akhercha Sep 27, 2024 •

edited

Loading

cchudant Sep 23, 2024

akhercha Sep 27, 2024 •

edited

Loading

cchudant Sep 23, 2024

akhercha Sep 27, 2024

cchudant commented Sep 23, 2024

cchudant commented Sep 23, 2024

antiyro commented Sep 25, 2024

akhercha commented Sep 27, 2024 •

edited

Loading

antiyro Sep 27, 2024

akhercha Sep 27, 2024

antiyro Sep 27, 2024

akhercha Sep 27, 2024

antiyro left a comment

antiyro Sep 27, 2024

akhercha Sep 27, 2024

akhercha left a comment

fix(cli): Fix and clean client modes and fetch presets #279

fix(cli): Fix and clean client modes and fetch presets #279

Conversation

antiyro commented Sep 23, 2024

cchudant left a comment • 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

akhercha Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akhercha Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cchudant commented Sep 23, 2024

cchudant commented Sep 23, 2024

antiyro commented Sep 25, 2024

akhercha commented Sep 27, 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

antiyro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akhercha left a comment

Choose a reason for hiding this comment

cchudant left a comment •

edited

Loading

akhercha Sep 27, 2024 •

edited

Loading

akhercha Sep 27, 2024 •

edited

Loading

akhercha commented Sep 27, 2024 •

edited

Loading