Global frombytearray #795

sebastianrof · 2024-12-15T16:36:25Z

No description provided.

…l-frombytearray

sebastianrof · 2024-12-18T09:26:37Z

@SethDusek i fixed the clippy and rustfmt errors in the workflow

SethDusek

Hi, thanks for the contribution. Please see my comments

SethDusek · 2024-12-19T04:34:09Z

ergotree-ir/src/types/sglobal.rs

+        tpe: SFunc {
+            t_dom: vec![SType::SGlobal, SType::SColl(SType::SByte.into())],
+            t_range:SType::STypeVar(STypeVar::t()).into(),
+            tpe_params: vec![STypeParam::param_t()],


I don't think this is the right way to do this. Since the output type of fromBigEndianBytes can't be inferred from input, the output type needs to be explicitly serialized/deserialized, so add it to explicit_type_args list, see:

sigma-rust/ergotree-ir/src/types/sbox.rs

Line 60 in f6048c1

explicit_type_args: vec![STypeVar::t()]

Also, can you add roundtrip tests for MethodCalls using this new method? You can skip versioning for now as versioning semantics are still not final in sigmastate-interpreter

Thanks for the review. I'll add explicit type args for proper type inference and include proptest roundtrip tests for MethodCall.

SethDusek · 2024-12-19T04:35:14Z

ergotree-interpreter/src/eval/sglobal.rs

+                ));
+            }
+            let bytes_vec: Vec<u8> = bytes.iter().map(|&x| x as u8).collect();
+            let big_int = num_bigint::BigInt::from_bytes_be(num_bigint::Sign::Plus, &bytes_vec);


This would make it impossible to deserialize negative bigints, I suggest using BigInt256::from_be_slice instead

Using BigInt256::from_be_slice now to handle negative bigints correctly.

SethDusek · 2024-12-19T04:37:17Z

ergotree-interpreter/src/eval/sglobal.rs

+                    "To deserialize Int with fromBigEndianBytes, exactly four bytes should be provided".to_string(),
+                ));
+            }
+            let bytes_vec: Vec<u8> = bytes.iter().map(|&x| x as u8).collect();


Better to do this without heap allocating, rust doesn't have any great ways to collect into fixed-size arrays, so maybe a fold that accumulates bytes would be better: something like bytes.iter().map(|b| b as u8).fold(0i32, |acc, x| (acc << 8) || x as i32)

Fixed to use fold for heap-free byte accumulation.

SethDusek · 2024-12-19T04:45:46Z

ergotree-interpreter/src/eval/sglobal.rs

@@ -98,4 +196,98 @@ mod tests {
            expected_xor.as_slice()
        );
    }
+
+    #[test]


Can you add roundtrip tests (for example i64->big-endian byte array->back to i64 using this method) for arbitrary bytes/shorts/etc using proptest? See other proptest examples like in src/eval/downcast.rs

Added proptest roundtrip tests following the pattern in downcast.rs.

SethDusek · 2024-12-19T04:46:17Z

ergotree-ir/src/types/stype_param.rs

@@ -100,3 +100,14 @@ pub struct STypeParam {
    upper_bound: Option<SType>,
    lower_bound: Option<SType>,
 }
+
+impl STypeParam {


Don't think this is needed, check comments in sglobal.rs

you are right. i removed it.

SethDusek · 2024-12-19T04:49:14Z

ergotree-interpreter/src/eval/sglobal.rs

+        )
+        .unwrap()
+        .into();
+        let ctx = force_any_val::<Context>();


Consider using eval_out_wo_ctx instead for clarity

coveralls · 2024-12-19T05:20:15Z

Pull Request Test Coverage Report for Build 12515526296

Details

33 of 49 (67.35%) changed or added relevant lines in 3 files are covered.
18 unchanged lines in 4 files lost coverage.
Overall coverage decreased (-0.1%) to 78.334%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
ergotree-ir/src/types/sglobal.rs	7	8	87.5%
ergotree-interpreter/src/eval/sglobal.rs	25	40	62.5%

Files with Coverage Reduction	New Missed Lines	%
ergo-p2p/src/peer_info.rs	1	68.42%
ergotree-ir/src/chain/token.rs	5	72.88%
ergotree-ir/src/serialization/types.rs	6	80.42%
ergo-p2p/src/peer_spec.rs	6	76.56%

Totals
Change from base Build 12422681236:	-0.1%
Covered Lines:	11031
Relevant Lines:	14082

💛 - Coveralls

sebastianrof · 2024-12-24T11:36:36Z

@SethDusek, I've made the requested changes. Could you please review again?

sebastianrof · 2024-12-25T15:05:32Z

@SethDusek i fix the build without default features issue faced in the pipeline.

SethDusek · 2024-12-25T10:48:29Z

ergotree-interpreter/src/eval/sglobal.rs

+                })?,
+            ))
+        }
+        SType::SUnit => {


The original implementation does not have a case for SUnit

Yes, Removed it.

sebastianrof · 2024-12-27T10:57:08Z

@SethDusek I removed the SUint additional type

SethDusek

Looks good, but can you please squash your last 4 commits into one?

sebastianrof · 2024-12-27T11:31:06Z

Looks good, but can you please squash your last 4 commits into one?

Done.

sebastianrof added 2 commits December 15, 2024 19:54

adding the global.frombytearraybigendian

faf6349

Merge remote-tracking branch 'origin/global-frombytearray' into globa…

4173f4b

…l-frombytearray

sebastianrof mentioned this pull request Dec 15, 2024

Add Global.fromBigEndianBytes #786

Open

Merge branch 'ergoplatform:develop' into global-frombytearray

038ec0d

SethDusek requested changes Dec 19, 2024

View reviewed changes

sebastianrof requested a review from SethDusek December 20, 2024 11:19

SethDusek requested changes Dec 27, 2024

View reviewed changes

sebastianrof requested a review from SethDusek December 27, 2024 10:55

SethDusek approved these changes Dec 27, 2024

View reviewed changes

Fixing the issues

c1022e7

sebastianrof force-pushed the global-frombytearray branch from f706a18 to c1022e7 Compare December 27, 2024 11:29

sebastianrof requested a review from SethDusek December 27, 2024 11:31

SethDusek merged commit 5ce27fd into ergoplatform:develop Dec 27, 2024
10 of 12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Global frombytearray #795

Global frombytearray #795

sebastianrof commented Dec 15, 2024

sebastianrof commented Dec 18, 2024

SethDusek left a comment

SethDusek Dec 19, 2024

sebastianrof Dec 19, 2024

SethDusek Dec 19, 2024

sebastianrof Dec 19, 2024

SethDusek Dec 19, 2024

sebastianrof Dec 19, 2024

SethDusek Dec 19, 2024

sebastianrof Dec 19, 2024

SethDusek Dec 19, 2024

sebastianrof Dec 19, 2024

SethDusek Dec 19, 2024

sebastianrof Dec 19, 2024

coveralls commented Dec 19, 2024 •

edited

Loading

sebastianrof commented Dec 24, 2024

sebastianrof commented Dec 25, 2024

SethDusek Dec 25, 2024

sebastianrof Dec 27, 2024

sebastianrof commented Dec 27, 2024

SethDusek left a comment

sebastianrof commented Dec 27, 2024

Global frombytearray #795

Global frombytearray #795

Conversation

sebastianrof commented Dec 15, 2024

sebastianrof commented Dec 18, 2024

SethDusek 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

Choose a reason for hiding this comment

coveralls commented Dec 19, 2024 • edited Loading

Pull Request Test Coverage Report for Build 12515526296

Details

💛 - Coveralls

sebastianrof commented Dec 24, 2024

sebastianrof commented Dec 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastianrof commented Dec 27, 2024

SethDusek left a comment

Choose a reason for hiding this comment

sebastianrof commented Dec 27, 2024

coveralls commented Dec 19, 2024 •

edited

Loading