feat(config): struct pointers

[yair-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[yair-starkware]

• chore(gateway,batcher): make gw and batcher use same values for execution #1954
• feat(config): struct pointers #1953 👈 (View in Graphite)
• refactor(config): remove separation of required and non required config pointers #2111
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[codecov]

Codecov Report

Attention: Patch coverage is 93.87755% with 3 lines in your changes missing coverage. Please review.

Project coverage is 69.37%. Comparing base (e3165c4) to head (467ccb1).
Report is 454 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/papyrus_config/src/dumping.rs	95.74%	2 Missing ⚠️
crates/papyrus_config/src/lib.rs	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1953       +/-   ##
===========================================
+ Coverage   40.10%   69.37%   +29.27%     
===========================================
  Files          26        7       -19     
  Lines        1895      676     -1219     
  Branches     1895      676     -1219     
===========================================
- Hits          760      469      -291     
+ Misses       1100      178      -922     
+ Partials       35       29        -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Itay-Tsabary-Starkware]

Reviewable status: 0 of 3 files reviewed, 6 unresolved discussions (waiting on @yair-starkware)

---

crates/papyrus_config/src/lib.rs line 153 at r2 (raw file):

/// A serialized type of a configuration parameter.
#[derive(Clone, Copy, Serialize, Deserialize, Debug, PartialEq, strum_macros::Display)]
#[allow(missing_docs)]

Is this still needed?

Code quote:

#[allow(missing_docs)]

---

crates/papyrus_config/src/dumping.rs line 66 at r2 (raw file):

pub type ConfigPointers = Vec<(PointerTarget, Pointers)>;

/// Given set of paths that are configuration of the same struct type, makes all the paths point to

Given a set

Code quote:

Given set 

---

crates/papyrus_config/src/dumping.rs line 75 at r2 (raw file):

) {
    let target_dump = default_instance.dump();
    for (param_path, serialized_param) in target_dump {

Could this be default_instance.dump(), and then you wouldn't have to define target_dump?

Code quote:

target_dump

---

crates/papyrus_config/src/dumping.rs line 86 at r2 (raw file):

                    .strip_prefix(REQUIRED_PARAM_DESCRIPTION_PREFIX)
                    .unwrap_or(&serialized_param.description);
                ser_pointer_target_required_param(

Note this sets the target as a required parameter. In https://reviewable.io/reviews/starkware-libs/sequencer/1954 however it is added as a non-required parameter.
I'd be more explicit here, and have two variants, one for required and one for non-required params. They can both call a common base function if that makes sense.

Code quote:

ser_pointer_target_required_param(

---

crates/papyrus_config/src/dumping.rs line 92 at r2 (raw file):

                )
            }
            SerializedContent::PointerTarget(_) => panic!("Pointer target cannot be a pointer."),

Why not point it at the target as well?

Code quote:

SerializedContent::PointerTarget(_) => panic!("Pointer target cannot be a pointer."),

---

crates/papyrus_config/src/dumping.rs line 98 at r2 (raw file):

            pointer_prefixes
                .iter()
                .map(|pointer| format!("{}{}{}", pointer, FIELD_SEPARATOR, param_path))

Wrap as a function?

Code quote:

format!("{}{}{}", pointer, FIELD_SEPARATOR, param_path)

---

crates/papyrus_config/src/dumping.rs line 100 at r2 (raw file):

                .map(|pointer| format!("{}{}{}", pointer, FIELD_SEPARATOR, param_path))
                .collect(),
        ));

Why not create a vector and return it?

Code quote:

        config_pointers.push((
            pointer_target,
            pointer_prefixes
                .iter()
                .map(|pointer| format!("{}{}{}", pointer, FIELD_SEPARATOR, param_path))
                .collect(),
        ));

[yair-starkware]

Reviewable status: 0 of 3 files reviewed, 6 unresolved discussions (waiting on @Itay-Tsabary-Starkware)

---

crates/papyrus_config/src/dumping.rs line 66 at r2 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Given a set

Done.

---

crates/papyrus_config/src/dumping.rs line 75 at r2 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Could this be default_instance.dump(), and then you wouldn't have to define target_dump?

Done.

---

crates/papyrus_config/src/dumping.rs line 86 at r2 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Note this sets the target as a required parameter. In https://reviewable.io/reviews/starkware-libs/sequencer/1954 however it is added as a non-required parameter.
I'd be more explicit here, and have two variants, one for required and one for non-required params. They can both call a common base function if that makes sense.

Why do we have separation if they are combined later?

---

crates/papyrus_config/src/dumping.rs line 92 at r2 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Why not point it at the target as well?

I don't think we support pointer to pointer, did you try it?

---

crates/papyrus_config/src/dumping.rs line 100 at r2 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Why not create a vector and return it?

Done.

---

crates/papyrus_config/src/lib.rs line 153 at r2 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Is this still needed?

Yes, otherwise you need to doc each variant

[yair-starkware]

crates/papyrus_config/src/dumping.rs line 92 at r2 (raw file):

Previously, yair-starkware (Yair) wrote…

I don't think we support pointer to pointer, did you try it?

Thinking about it, the dump of a struct shouldn't contain pointers at all I think, only in the dump_to_file it should get pointers

[Itay-Tsabary-Starkware]

Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @yair-starkware)

---

crates/papyrus_config/src/dumping.rs line 86 at r2 (raw file):

Previously, yair-starkware (Yair) wrote…

Why do we have separation if they are combined later?

You could define a required parameter that no one points at. That's the current usage in papyrus.
However, we might as well just create required parameters as targets and have an empty set of pointers.
WDYT?

---

crates/papyrus_config/src/dumping.rs line 100 at r2 (raw file):

Previously, yair-starkware (Yair) wrote…

Done.

I had something else in mind, wdyt?

The current code structure is:

let mut res;
loop over some list {
    ....
    calculations
    ....
   add a single result to res
}
res

I suggest to change it such it is more of a map-like syntax, i.e., map each element from some list to an element in res.

[yair-starkware]

Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @Itay-Tsabary-Starkware)

---

crates/papyrus_config/src/dumping.rs line 86 at r2 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

You could define a required parameter that no one points at. That's the current usage in papyrus.
However, we might as well just create required parameters as targets and have an empty set of pointers.
WDYT?

Consolidated pointers as discussed in #2111

---

crates/papyrus_config/src/dumping.rs line 100 at r2 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

I had something else in mind, wdyt?

The current code structure is:

let mut res;
loop over some list {
    ....
    calculations
    ....
   add a single result to res
}
res

I suggest to change it such it is more of a map-like syntax, i.e., map each element from some list to an element in res.

Done.

[Itay-Tsabary-Starkware]

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @yair-starkware)

---

crates/papyrus_config/src/dumping.rs line 87 at r4 (raw file):

// Converts a serialized param to a pointer target.
fn to_target(

Please change to a more descriptive name, e.g., serialized_param_to_pointer_target (leaving as non blocking).

Code quote:

to_target

[yair-starkware]

Reviewable status: 0 of 3 files reviewed, all discussions resolved (waiting on @Itay-Tsabary-Starkware)

---

crates/papyrus_config/src/dumping.rs line 87 at r4 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Please change to a more descriptive name, e.g., serialized_param_to_pointer_target (leaving as non blocking).

done

[Itay-Tsabary-Starkware]

Reviewed 1 of 3 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @yair-starkware)

[yair-starkware]

Merge activity

• Nov 18, 4:13 AM EST: A user started a stack merge that includes this pull request via Graphite.
• Nov 18, 4:15 AM EST: Graphite rebased this pull request as part of a merge.
• Nov 18, 4:35 AM EST: A user merged this pull request with Graphite.