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.

Sign up for GitHub

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

Jump to bottom

Error handling #133

Closed

faytey wants to merge 0 commits into lambdaclass:main from faytey:error_handling

faytey commented Oct 8, 2023

A draft of the error-handling techniques used for testing purposes, kindly let me know if the method used so far is acceptable. Thanks

faytey requested review from rcatalan98, dpinones and dubzn as code owners

October 8, 2023 17:14

uri-99 requested changes

View reviewed changes

Collaborator

uri-99 left a comment

some format errors. You should run make fmt before pushing

src/contracts/yas_pool.cairo Outdated

@@ @@ -25,7 +25,8 @@ trait IYASPool<TContractState> { @@
               #[starknet::contract]
               mod YASPool {
-                  use super::IYASPool;
+                  use core::result::ResultTrait;
+              use super::IYASPool;

Collaborator

uri-99 Oct 19, 2023

format error. You should run make fmt before pushing

src/libraries/tick.cairo Outdated

@@ @@ -74,7 +73,8 @@ trait ITick<TContractState> { @@
               #[starknet::contract]
               mod Tick {
-                  use super::{ITick, Info};
+                  use core::result::ResultTrait;
+              use super::{ITick, Info};

Collaborator

uri-99 Oct 19, 2023

format error. You should run make fmt before pushing

src/libraries/tick_bitmap.cairo Outdated

@@ @@ -10,7 +10,8 @@ trait ITickBitmap<TContractState> { @@
               #[starknet::contract]
               mod TickBitmap {
-                  use super::ITickBitmap;
+                  use core::result::ResultTrait;
+              use super::ITickBitmap;

Collaborator

uri-99 Oct 19, 2023

format error. You should run make fmt before pushing

src/tests/test_libraries/test_bit_math.cairo Outdated

@@ @@ -1,81 +1,131 @@ @@
               mod BitMathTests {
                   mod MostSignificantBit {
-                      use integer::BoundedInt;
+                      use core::result::ResultTrait;
+              use integer::BoundedInt;

Collaborator

uri-99 Oct 19, 2023

format error. You should run make fmt before pushing

src/tests/test_libraries/test_bit_math.cairo Outdated

                       }
                   }
                   mod LeastSignificantBit {
-                      use integer::BoundedInt;
+                      use core::result::ResultTrait;
+              use integer::BoundedInt;

Collaborator

uri-99 Oct 19, 2023

format error. You should run make fmt before pushing

uri-99 requested changes

View reviewed changes

Collaborator

uri-99 left a comment

some missing #[available_gas()] attributes in #[tests]. It is important to evaluate gas consumption, to ensure there are no deadlocks.

src/tests/test_libraries/test_bit_math.cairo Outdated

                       use yas::libraries::bit_math::BitMath::most_significant_bit;
                       #[test]
-                      fn msb_happy_path() {
+                      fn msb_happy_path() -> Result<u8, felt252> {

Collaborator

uri-99 Oct 19, 2023

#[test] should contain #[available_gas()] attribute

Suggested change

      
                    fn msb_happy_path() -> Result<u8, felt252> {
          
                    #[available_gas(200000000)]
          
                    fn msb_happy_path() -> Result<u8, felt252> {

src/tests/test_libraries/test_bit_math.cairo Outdated

Comment on lines 17 to 18

                       #[test]
-                      fn msb_larger_number() {
+                      fn msb_larger_number() -> Result<u8, felt252> {

Collaborator

uri-99 Oct 19, 2023

#[test] should contain #[available_gas()] attribute

src/tests/test_libraries/test_bit_math.cairo Outdated

Comment on lines 27 to 28

                       #[test]
-                      fn msb_bigger_number() {
+                      fn msb_bigger_number() -> Result<u8, felt252> {

Collaborator

uri-99 Oct 19, 2023

#[test] should contain #[available_gas()] attribute

src/tests/test_libraries/test_bit_math.cairo Outdated

Comment on lines 38 to 39

                       #[test]
-                      fn msb_maximum_256() {
-                          assert(most_significant_bit(BoundedInt::max()) == 255, 'msb should be 255');
+                      fn msb_maximum_256() -> Result<u8, felt252> {

Collaborator

uri-99 Oct 19, 2023

#[test] should contain #[available_gas()] attribute

src/tests/test_libraries/test_bit_math.cairo Outdated

Comment on lines 48 to 49

                       #[test]
-                      fn msb_random_number() {
+                      fn msb_random_number() -> Result<u8, felt252> {

Collaborator

uri-99 Oct 19, 2023

#[test] should contain #[available_gas()] attribute

src/tests/test_libraries/test_bit_math.cairo Outdated

Comment on lines 82 to 83

                       #[test]
-                      fn lsb_larger_number() {
+                      fn lsb_larger_number() -> Result<u8, felt252> {

Collaborator

uri-99 Oct 19, 2023

#[test] should contain #[available_gas()] attribute

src/tests/test_libraries/test_bit_math.cairo Outdated

Comment on lines 93 to 94

                       #[test]
-                      fn lsb_bigger_number() {
+                      fn lsb_bigger_number() -> Result<u8, felt252> {

Collaborator

uri-99 Oct 19, 2023

#[test] should contain #[available_gas()] attribute

src/tests/test_libraries/test_bit_math.cairo Outdated

Comment on lines 104 to 105

                       #[test]
-                      fn lsb_maximum_256() {
+                      fn lsb_maximum_256() -> Result<u8, felt252> {

Collaborator

uri-99 Oct 19, 2023

#[test] should contain #[available_gas()] attribute

src/tests/test_libraries/test_bit_math.cairo Outdated

Comment on lines 115 to 116

                       #[test]
-                      fn lsb_random_number() {
+                      fn lsb_random_number() -> Result<u8, felt252> {

Collaborator

uri-99 Oct 19, 2023

#[test] should contain #[available_gas()] attribute

src/tests/test_libraries/test_bit_math.cairo Outdated

Comment on lines 127 to 129

                       #[test]
-                      #[should_panic]
+                      // #[should_panic]
                       fn lsb_number_zero() {

Collaborator

uri-99 Oct 19, 2023

#[test] should contain #[available_gas()] attribute

uri-99 requested changes

View reviewed changes

Collaborator

uri-99 left a comment

Tests should assert() different values regarding the functionality being tested; to ensure the functionality fails when it should fail and also to ensure that it doesn't fail when it shouldn't

src/tests/test_libraries/test_bit_math.cairo Outdated

+                          if !(most_significant_bit(1).expect('msb errored') == 0 ) {
+                              return Result::Err('msb should be 0');
+                          }
+                          return most_significant_bit(1);

Collaborator

uri-99 Oct 19, 2023

Why does a Test return a value?

src/tests/test_libraries/test_bit_math.cairo Outdated

+                          if !(most_significant_bit(128).expect('msb errored') == 7 ) {
+                              return Result::Err('msb should be 7');
+                          }
+                          return most_significant_bit(128);

Collaborator

uri-99 Oct 19, 2023

Why does a Test return a value?

src/tests/test_libraries/test_bit_math.cairo Outdated

+                          if !(most_significant_bit(1000000).expect('msb errored') == 19) {
+                              return Result::Err('msb should be 19');
+                          }
+                          return most_significant_bit(1000000);

Collaborator

uri-99 Oct 19, 2023

Why does a Test return a value?

src/tests/test_libraries/test_bit_math.cairo Outdated

+                          if !(most_significant_bit(BoundedInt::max()).expect('msb errored') == 255) {
+                              return Result::Err('msb should be 255');
+                          }
+                          return most_significant_bit(BoundedInt::max());

Collaborator

uri-99 Oct 19, 2023

Why does a Test return a value?

src/tests/test_libraries/test_bit_math.cairo Outdated

+                          if !(most_significant_bit(12345).expect('msb errored') == 13) {
+                              return Result::Err('msb should be 13');
+                          }
+                          return most_significant_bit(12345);

Collaborator

uri-99 Oct 19, 2023

Why does a Test return a value?

src/tests/test_libraries/test_bit_math.cairo Outdated

+                          if !(ret.expect('lsb errored') == 0) {
+                              return Result::Err('lsb should be 0');
+                          }
+                          return ret;

Collaborator

uri-99 Oct 19, 2023

Why does a Test return a value?

src/tests/test_libraries/test_liquidity_math.cairo Outdated

+                  if !(z == 15) {
+                      return Result::Err('z == 15');
+                  }
+                  return Result::Ok(z);

Collaborator

uri-99 Oct 19, 2023

Why does a Test return a value?

src/tests/test_libraries/test_liquidity_math.cairo Outdated

+                  if !(z == 2) {
+                      return Result::Err('z == 2');
+                  }
+                  return Result::Ok(z);

Collaborator

uri-99 Oct 19, 2023

Why does a Test return a value?

src/tests/test_libraries/test_liquidity_math.cairo Outdated

+                  if !(z == 2) {
+                      return Result::Err('z == 2');
+                  }
+                  return Result::Ok(z);

Collaborator

uri-99 Oct 19, 2023

Why does a Test return a value?

src/tests/test_libraries/test_liquidity_math.cairo Outdated

+                  if !(z == 2) {
+                      return Result::Err('z == 2');
+                  }
+                  return Result::Ok(z);

Collaborator

uri-99 Oct 19, 2023

Why does a Test return a value?

Author

faytey commented Oct 19, 2023

So maybe I got the task wrong, I was asked to handle errors and replace all asserts, also to use the Result method for error handling. Please can you help explain the task properly so I do the correct thing please thanks

Collaborator

rcatalan98 commented Oct 20, 2023

So maybe I got the task wrong, I was asked to handle errors and replace all asserts, also to use the Result method for error handling. Please can you help explain the task properly so I do the correct thing please thanks

Hi @faytey! No problem maybe we weren't clear enough with the issue. The task is to refactor the listed function to return Result instead of having the assert in this library functions. So for testing this functions you will receive whatever the function returns and use patter matching to check if there was an error or not. This same strategy should be used by any function that calls the underlying library functions.

Is that clear?

faytey closed this

faytey force-pushed the error_handling branch from 0421a94 to 88a08d1 Compare

October 31, 2023 12:34

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet