Refactor(eth-connector): Use Result return values instead of panicking #355

birchmd · 2021-11-16T21:32:17Z

The purpose of this PR is to remove panic calls (sdk_unwrap, assert, etc) from the connector logic. The reasons for this are:

Continue to remove references to NEAR host functions from core logic. When we have removed all the host functions then we will be able to compile a standalone binary from the engine (as required for the standalone engine project).
Panics should only happen at the edge of applications (unless they are fatal errors) where there is no other way to handle the error. In smart contract development this principle is often ignored (or alternatively all errors are considered fatal) because panicking in a contract only fails the current transaction. However in the standalone engine a panic will bring down the whole process which will be a big problem for the relayer waiting for an FFI response.
Explicitly defining errors in the types of functions can make it easier to understand the logic of a module (especially something complicated like the eth-connector). It is clear in the types which calls are fallible and why.

mrLSD · 2021-11-17T12:51:45Z

I have concerns about Option<...> type results instead of panics.
When we panic, we have details about the incident. For example:

assert_eq!(val.len(), 2);

will return details of what was wrong with length.
But if you just return None you lose that details, and I believe helpful to debug critical cases.

I suggest not to use Option as an error return at all, and not to ignore the errors that used to be in the assertion. And return Result type instead. It will extend our error flow.
Also detailed errors is good to debug incidents. Without this, out of the blue, we can create problems for ourselves.

About removing panics to error types - awesome idea!

aurora-is-near#355)

birchmd · 2021-11-17T13:37:23Z

Thanks for the suggestion @mrLSD . I have added error types for the message parsing functions.

joshuajbouw

So far, really good. Lots of QOL changes which is very welcomed. Just a few requests on my behalf.

engine/src/admin_controlled.rs

engine/src/connector.rs

engine/src/deposit_event.rs

engine/src/engine.rs

engine/src/lib.rs

birchmd · 2021-11-18T14:28:45Z

@joshuajbouw comments addresses. PTAL

#355)

mrLSD

@birchmd LGTM!
Minor comments, that can be added to some another PR.

mrLSD · 2021-11-18T16:33:40Z

engine/src/parameters.rs

+impl TryFrom<JsonValue> for StorageBalanceOfCallArgs {
+    type Error = error::ParseTypeFromJsonError;
+
+    fn try_from(v: JsonValue) -> Result<Self, error::ParseTypeFromJsonError> {


The correct way is: Result<Self, Self::Error>q

mrLSD · 2021-11-18T16:34:09Z

engine/src/parameters.rs

+impl TryFrom<JsonValue> for TransferCallCallArgs {
+    type Error = error::ParseTypeFromJsonError;
+
+    fn try_from(v: JsonValue) -> Result<Self, error::ParseTypeFromJsonError> {


The correct way is: Result<Self, Self::Error>q

mrLSD · 2021-11-18T16:50:10Z

engine/src/fungible_token.rs

@@ -234,6 +243,7 @@ impl<I: IO + Copy> FungibleTokenOps<I> {
        if let Some(memo) = memo {
            sdk::log(&crate::prelude::format!("Memo: {}", memo));


Is it correct? May be: sdk::log!?

* Feat(engine): London hard fork support (#244) * Fix(exit precompile): Address to refund in case of error is an argument (#311) * Feat(engine): Make engine parametric in storage access (#314) * Test verifying the EVM log returns the correct address (#341) * Remove sdk::current_account_id usage from engine-precompiles (#346) * Remove Default trait bound from engine IO (#342) * Remove some sdk usage from core logic (#347) * Factor out blockchain environment variable access as a trait (#349) * Factor out NEAR promise host functions into a trait (#353) * Borsh deserialized value field for call args (#351) * Refactor(eth-connector): Use Result return values instead of panicking (#355) * Gate all NEAR host functions behind the contract feature (#356) * Bump @openzeppelin/contracts from 4.3.2 to 4.3.3 in /etc/eth-contracts * Chore: Newtypes for gas (#344) * Feat(standalone): Standalone (#345) * Minor fixes to sdk refactor (#359) * Refactor(engine): Move submit logic into engine module (#366) * Feat(standalone): Storage backend (#375) * NEAR random numbers from solidity contract (#368) * Feat(standalone): EVM tracing via SputnikVM (#383) * Feat(standalone): Bootstrap storage from relayer and state snapshots (#379) * Feat(standalone): Structures and logic for keeping storage in sync with the blockchain (#382) * Feat(standalone): Capture geth-like tracing from SputnikVM events (#384) * Connector cleanup (#374) * Remove betanet * Fix(engine): original_storage bug fix; more tracing tests (#390) * Increase NEAR Gas for ft_on_transfer (#389) Co-authored-by: Andrew Bednoff <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Evgeny Ukhanov <[email protected]> Co-authored-by: Marcelo Fornet <[email protected]> Co-authored-by: Michael Birch <[email protected]>

birchmd added A-connector Area: Issues that relate to the connector. A-standalone Area: the standalone engine EVM labels Nov 16, 2021

birchmd requested review from mrLSD and joshuajbouw November 16, 2021 21:32

birchmd added a commit to birchmd/aurora-engine that referenced this pull request Nov 17, 2021

Refactor(eth-connector): Use Result return values instead of panicking (

b484c49

aurora-is-near#355)

birchmd force-pushed the sdk-refactor-panic branch from c1e40e6 to b484c49 Compare November 17, 2021 13:33

birchmd mentioned this pull request Nov 17, 2021

Gate all NEAR host functions behind the contract feature #356

Merged

joshuajbouw suggested changes Nov 18, 2021

View reviewed changes

joshuajbouw approved these changes Nov 18, 2021

View reviewed changes

Refactor(eth-connector): Use Result return values instead of panicking (

3744d21

#355)

birchmd force-pushed the sdk-refactor-panic branch from de0ccbd to 3744d21 Compare November 18, 2021 14:42

birchmd merged commit 3744d21 into develop Nov 18, 2021

birchmd deleted the sdk-refactor-panic branch November 18, 2021 14:45

mrLSD reviewed Nov 18, 2021

View reviewed changes

birchmd mentioned this pull request Nov 18, 2021

Minor fixes to sdk refactor #359

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor(eth-connector): Use Result return values instead of panicking #355

Refactor(eth-connector): Use Result return values instead of panicking #355

birchmd commented Nov 16, 2021

mrLSD commented Nov 17, 2021

birchmd commented Nov 17, 2021

joshuajbouw left a comment

birchmd commented Nov 18, 2021

mrLSD left a comment

mrLSD Nov 18, 2021

mrLSD Nov 18, 2021

mrLSD Nov 18, 2021

		@@ -234,6 +243,7 @@ impl<I: IO + Copy> FungibleTokenOps<I> {
		if let Some(memo) = memo {
		sdk::log(&crate::prelude::format!("Memo: {}", memo));

Refactor(eth-connector): Use Result return values instead of panicking #355

Refactor(eth-connector): Use Result return values instead of panicking #355

Conversation

birchmd commented Nov 16, 2021

mrLSD commented Nov 17, 2021

birchmd commented Nov 17, 2021

joshuajbouw left a comment

Choose a reason for hiding this comment

birchmd commented Nov 18, 2021

mrLSD left a comment

Choose a reason for hiding this comment

mrLSD Nov 18, 2021

Choose a reason for hiding this comment

mrLSD Nov 18, 2021

Choose a reason for hiding this comment

mrLSD Nov 18, 2021

Choose a reason for hiding this comment