Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add test for Bravos transfer #721

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Add test for Bravos transfer #721

wants to merge 9 commits into from

Conversation

FrancoGiachetta
Copy link
Contributor

@FrancoGiachetta FrancoGiachetta commented Jul 5, 2024

Refers to #717
The purpose of the PR is to add tests which reproduce similar cases of the tx.

Checklist

  • Linked to Github Issue
  • Unit tests added
  • Integration tests added.
  • This change requires new documentation.
    • Documentation has been added/updated.

@FrancoGiachetta FrancoGiachetta changed the title Fede debug Add test for Bravos transfer Jul 5, 2024
Copy link

github-actions bot commented Jul 5, 2024

Benchmark results Main vs HEAD.

Command Mean [s] Min [s] Max [s] Relative
head factorial_2M.cairo (JIT) 3.167 ± 0.022 3.135 3.197 1.01 ± 0.01
base factorial_2M.cairo (JIT) 3.196 ± 0.028 3.156 3.241 1.02 ± 0.01
head factorial_2M.cairo (AOT) 3.127 ± 0.020 3.095 3.162 1.00
base factorial_2M.cairo (AOT) 3.202 ± 0.043 3.131 3.257 1.02 ± 0.02
Command Mean [s] Min [s] Max [s] Relative
head fib_2M.cairo (JIT) 2.807 ± 0.043 2.747 2.888 1.04 ± 0.02
base fib_2M.cairo (JIT) 2.855 ± 0.032 2.778 2.890 1.06 ± 0.01
head fib_2M.cairo (AOT) 2.854 ± 0.053 2.776 2.907 1.06 ± 0.02
base fib_2M.cairo (AOT) 2.694 ± 0.022 2.663 2.730 1.00
Command Mean [s] Min [s] Max [s] Relative
head logistic_map.cairo (JIT) 3.020 ± 0.022 2.988 3.049 1.05 ± 0.01
base logistic_map.cairo (JIT) 3.060 ± 0.032 3.028 3.131 1.06 ± 0.02
head logistic_map.cairo (AOT) 2.902 ± 0.026 2.864 2.933 1.01 ± 0.01
base logistic_map.cairo (AOT) 2.877 ± 0.030 2.820 2.915 1.00

@FrancoGiachetta FrancoGiachetta marked this pull request as ready for review July 5, 2024 15:55
Copy link
Collaborator

@pefontana pefontana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, can you link the issue to the PR, like we do here #720

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this?

@@ -176,6 +176,23 @@ fn test_program_cases(program_path: &str) {
#[test_case("tests/cases/cairo_vm/contracts/field_sqrt.cairo", &[])]
#[test_case("tests/cases/cairo_vm/contracts/random_ec_point.cairo", &[])]
#[test_case("tests/cases/cairo_vm/contracts/alloc_constant_size.cairo", &[10, 10, 10])]
#[test_case("tests/cases/cairo_vm/contracts/multicall.cairo", &[
// n_calls
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add in the commented lines what these numbers are?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Further, maybe using the serialized bytes and include! results in something readable?

@@ -0,0 +1,35 @@
#[starknet::contract]
mod MultiCall {
// use starknet::account::Call;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check in the issue,

In the multicall.cairo, there are some commented lines. If you change the type of calldata to Span, uncomment those lines and comment out the ones above so that it compiles.

Can we add the two cases of this contract with the two different data types?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pefontana which is the other calldata's type you are refering to? Because its type was Span when I first saw it.

@codecov-commenter
Copy link

codecov-commenter commented Jul 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.15%. Comparing base (c866fbf) to head (9ba1c94).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #721      +/-   ##
==========================================
+ Coverage   87.98%   88.15%   +0.16%     
==========================================
  Files         122      122              
  Lines       34289    34289              
==========================================
+ Hits        30169    30226      +57     
+ Misses       4120     4063      -57     

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

@@ -9,7 +9,7 @@ use test_case::test_case;

// Test cases for programs without input, it checks the outputs are correct automatically.

// felt tests
// // felt tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// // felt tests
// felt tests

@azteca1998 azteca1998 linked an issue Jul 16, 2024 that may be closed by this pull request
Copy link

Benchmarking results

Benchmark for program factorial_2M

Open benchmarks
Command Mean [s] Min [s] Max [s] Relative
Cairo-vm (Rust, Cairo 1) 10.634 ± 0.240 10.430 11.152 23.68 ± 0.55
cairo-native (embedded AOT) 3.135 ± 0.046 3.095 3.251 6.98 ± 0.11
cairo-native (embedded JIT using LLVM's ORC Engine) 3.171 ± 0.018 3.150 3.204 7.06 ± 0.05
cairo-native (standalone AOT) 0.674 ± 0.001 0.672 0.676 1.50 ± 0.01
cairo-native (standalone AOT with -march=native) 0.449 ± 0.002 0.448 0.455 1.00

Benchmark for program fib_2M

Open benchmarks
Command Mean [s] Min [s] Max [s] Relative
Cairo-vm (Rust, Cairo 1) 10.759 ± 0.325 10.312 11.111 1292.03 ± 40.20
cairo-native (embedded AOT) 2.655 ± 0.017 2.633 2.681 318.78 ± 3.15
cairo-native (embedded JIT using LLVM's ORC Engine) 2.673 ± 0.013 2.649 2.693 321.04 ± 2.88
cairo-native (standalone AOT) 0.008 ± 0.000 0.008 0.009 1.00
cairo-native (standalone AOT with -march=native) 0.008 ± 0.002 0.008 0.027 1.01 ± 0.24

Benchmark for program logistic_map

Open benchmarks
Command Mean [s] Min [s] Max [s] Relative
Cairo-vm (Rust, Cairo 1) 4.257 ± 0.036 4.209 4.318 59.66 ± 2.88
cairo-native (embedded AOT) 2.810 ± 0.017 2.781 2.829 39.37 ± 1.89
cairo-native (embedded JIT using LLVM's ORC Engine) 2.977 ± 0.018 2.946 3.006 41.72 ± 2.00
cairo-native (standalone AOT) 0.115 ± 0.000 0.114 0.115 1.60 ± 0.08
cairo-native (standalone AOT with -march=native) 0.071 ± 0.003 0.070 0.092 1.00

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Bravos transfer minimal contract test
6 participants