Parsing of Individual section of random values #129

Fumuran · 2023-01-30T15:18:14Z

Partly addressing #86.
Implementing parsing of random values, declared in individual section.

grjte

Thanks @Overcastan, this looks good! I left a few thoughts inline

air-script/tests/aux_trace/aux_trace.air

air-script/tests/aux_trace/aux_trace.rs

air-script/tests/variables/variables.air

air-script/tests/variables/variables.rs

parser/src/lexer/tests/random_values.rs

parser/src/parser/grammar.lalrpop

grjte · 2023-01-30T17:32:56Z

parser/src/ast/random_values.rs

+/// Declaration of a random value used in [RandomValues]. It is represented by a named identifier and its size.
+#[derive(Debug, Eq, PartialEq)]
+pub struct RandBinding {
+    name: Identifier,
+    size: usize,
+}


Although binding is a good word choice, this feels inconsistent with the other ways we've implemented and named things. I wonder if we should either change this to NamedRandomValues or plan to use the word binding more consistently in other places.

Curious what others think

We can revisit this in a future PR, alongside @tohrnii's comment #129 (comment)

parser/src/parser/tests/random_values.rs

tohrnii

Thank you @Overcastan, overall looks good to me. I just left some small things inline.

parser/src/ast/random_values.rs

tohrnii · 2023-01-30T21:05:58Z

parser/src/ast/random_values.rs

+#[derive(Debug, Eq, PartialEq, Clone)]
+pub struct RandBinding {
+    name: Identifier,
+    size: u64,
+}
+
+impl RandBinding {
+    pub(crate) fn new(name: Identifier, size: u64) -> Self {
+        Self { name, size }
+    }
+
+    pub fn name(&self) -> &str {
+        let Identifier(name) = &self.name;
+        name
+    }
+
+    pub fn size(&self) -> u64 {
+        self.size
+    }
+}


I'm wondering if we can create a structure with more generic name that we can use for other similar structs (like TraceCols and PublicInput), and then define aliases for those instead. This will make things a little less redundant. cc: @grjte

I've thought about it too, but I haven't figured out how to do it well. It will be great if we can implement this

I agree that this would be ideal. Let me take a look and see if I think of anything. Otherwise, we can make a separate issue to revisit this in a future PR. It would be great to do it now, though.

Can we call this just Array or ArrayDecl?

Let's revisit this in a future PR

parser/src/error.rs

parser/src/parser/grammar.lalrpop

tohrnii

Thank you @Overcastan, looks good to me. I just left a few very minor nits.

parser/src/ast/random_values.rs

parser/src/error.rs

grjte

Thanks @Overcastan, looks good! Just a couple very small final things

parser/src/parser/grammar.lalrpop

parser/src/parser/tests/random_values.rs

grjte · 2023-02-01T13:57:48Z

parser/src/ast/random_values.rs

+#[derive(Debug, Eq, PartialEq, Clone)]
+pub struct RandBinding {
+    name: Identifier,
+    size: u64,
+}
+
+impl RandBinding {
+    pub(crate) fn new(name: Identifier, size: u64) -> Self {
+        Self { name, size }
+    }
+
+    pub fn name(&self) -> &str {
+        let Identifier(name) = &self.name;
+        name
+    }
+
+    pub fn size(&self) -> u64 {
+        self.size
+    }
+}


Let's revisit this in a future PR

grjte · 2023-02-01T13:58:28Z

parser/src/ast/random_values.rs

+/// Declaration of a random value used in [RandomValues]. It is represented by a named identifier and its size.
+#[derive(Debug, Eq, PartialEq)]
+pub struct RandBinding {
+    name: Identifier,
+    size: usize,
+}


We can revisit this in a future PR, alongside @tohrnii's comment #129 (comment)

Al-Kindi-0 · 2023-02-01T14:31:55Z

parser/src/ast/mod.rs

@@ -32,6 +35,7 @@ pub struct Source(pub Vec<SourceSection>);
 /// - PeriodicColumns: Periodic columns are each represented by a fixed-size array with all of its
 ///   elements specified. The array length is expected to be a power of 2, but this is not checked
 ///   during parsing.
+/// - RandomValues: Random Values represent the randomness sent by the Verifier


nit: a dot at the end is missing

Al-Kindi-0

Looks great, thank you @Overcastan !

grjte

LGTM! Thank you!

Fumuran force-pushed the andrew-parse-random-values branch from 52c0aa1 to 9bbf0b9 Compare January 30, 2023 15:20

Fumuran requested review from grjte, tohrnii, bobbinth and Al-Kindi-0 January 30, 2023 15:20

grjte mentioned this pull request Jan 30, 2023

Add section for random values and allow declaring names for accessing random values #86

Closed

grjte suggested changes Jan 30, 2023

View reviewed changes

Fumuran requested a review from grjte January 30, 2023 20:13

tohrnii suggested changes Jan 30, 2023

View reviewed changes

Fumuran force-pushed the andrew-parse-random-values branch from 6d2eafb to 545b997 Compare January 30, 2023 22:36

tohrnii mentioned this pull request Jan 31, 2023

Tracking PR for v0.2 release #79

Merged

14 tasks

Fumuran force-pushed the andrew-parse-random-values branch 2 times, most recently from a471852 to 2a439dd Compare January 31, 2023 22:30

Fumuran requested a review from tohrnii January 31, 2023 22:33

tohrnii approved these changes Feb 1, 2023

View reviewed changes

parser/src/ast/random_values.rs Outdated Show resolved Hide resolved

parser/src/ast/random_values.rs Outdated Show resolved Hide resolved

parser/src/error.rs Outdated Show resolved Hide resolved

Fumuran force-pushed the andrew-parse-random-values branch from 2a439dd to 1c86ca1 Compare February 1, 2023 11:09

grjte removed the request for review from bobbinth February 1, 2023 12:17

grjte suggested changes Feb 1, 2023

View reviewed changes

feat: add parsing of rand values section, add parser tests

da6a45f

Fumuran force-pushed the andrew-parse-random-values branch from 1c86ca1 to da01d36 Compare February 1, 2023 14:25

Fumuran requested a review from grjte February 1, 2023 14:29

Al-Kindi-0 approved these changes Feb 1, 2023

View reviewed changes

grjte approved these changes Feb 1, 2023

View reviewed changes

feat: code refactor and tests improvement

6075ea5

Fumuran force-pushed the andrew-parse-random-values branch from da01d36 to 6075ea5 Compare February 1, 2023 14:48

grjte merged commit 6446fd2 into next Feb 2, 2023

grjte deleted the andrew-parse-random-values branch February 2, 2023 08:23

tohrnii mentioned this pull request Feb 7, 2023

Individual section and named access for random values #128

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parsing of Individual section of random values #129

Parsing of Individual section of random values #129

Fumuran commented Jan 30, 2023

grjte left a comment

grjte Jan 30, 2023

grjte Feb 1, 2023

tohrnii left a comment

tohrnii Jan 30, 2023

Fumuran Jan 30, 2023

grjte Jan 31, 2023

tohrnii Jan 31, 2023

grjte Feb 1, 2023

tohrnii left a comment

grjte left a comment

grjte Feb 1, 2023

grjte Feb 1, 2023

Al-Kindi-0 Feb 1, 2023

Al-Kindi-0 left a comment

grjte left a comment

Parsing of Individual section of random values #129

Parsing of Individual section of random values #129

Conversation

Fumuran commented Jan 30, 2023

grjte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tohrnii 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

tohrnii left a comment

Choose a reason for hiding this comment

grjte 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

Al-Kindi-0 left a comment

Choose a reason for hiding this comment

grjte left a comment

Choose a reason for hiding this comment