Improve the error handling in the implementation of the `storage_read` callback of the `NativeSyscallHandler`

[igaray]

The storage_read callback works but it's error handling has an unhandled case and includes dbg! macros which should at most log the error.

https://github.com/lambdaclass/starknet_in_rust/pull/943/files#diff-050b376c364b46aa56077588d77a5939973603c9caf47e452bc0895b5b6cd161R155

[Subbitoooo]

Hello man, Sorry if i see This Issue now but I'm texting you a possible solution. For the error handling of the storage_read callback within NativeSyscallHandler, it is crucial to address specific error types, such as FileNotFoundError, return distinct error codes for identification, log detailed information for debugging, handle permission issues, implement fallback mechanisms for critical operations, and document the error-handling strategy for clarity and understanding.
Here I'm writing for you an example and you should adapt it based on the specifics of your implementation and the requirements of Starknet Application
python
Copy code
def storage_read(file_path):
try:
with open(file_path, 'r') as file:
data = file.read()
# Process the data
return data
except FileNotFoundError:
# Handle file not found error
log_error(f"File not found: {file_path}")
return None
except PermissionError:
# Handle permission error
log_error(f"Permission error for file: {file_path}")
return None
except Exception as e:
# Handle other exceptions
log_error(f"An error occurred while reading {file_path}: {str(e)}")
return None

def log_error(message):
# Log the error with relevant details
print(f"ERROR: {message}")
# You can also log to a file or external logging service
Hope this will be helpful For Everyone, have a nice day :)

[fmoletta]

The unhandled case represents the case in which a storage read fails because of an external reason (such as failure to read from a database).
In this case returning an error is not the correct behaviour, as this will only propagate the error to the cairo program, making it seem like the error was due to the contract's logic rather than an external failure. As we are adding sandboxing for cairo-native execution.
Would it make sense to let it panick in this case? And let the sandbox handle it/fallback to cairo-vm. Otherwise this would require refactoring the trait itself to be able to "tell" the native context that the execution has failed due to an external failure.
What do you think about this @juanbono?

[juanbono]

I'm more in favour of returning a rust Error that can be handled as usual. Because panicking kinds of leak the information that Cairo Native can be run in a sandbox process (and this is not always the case).