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

Feat(standalone): logic for parsing TransactionKind from raw Near data #810

Merged
merged 4 commits into from
Aug 3, 2023

Conversation

birchmd
Copy link
Member

@birchmd birchmd commented Aug 2, 2023

Description

This is the first in a series of PRs that is meant to split up #705 . The idea is to merge the changes which are made in that PR in logical chunks until eventually the whole hashchain implementation is in. Doing the work in smaller pieces will both make it easier to review and prevent us from needing to maintain large, long-lived feature branches.

This first PR pulls in the transaction transaction parsing logic from borealis-engine-lib into this repo (in a future PR we will remove the duplicated code from borealis-engine-lib). The logic is used here to simplify how transactions are handled in tests because all transactions can automatically be passed to both the Near runtime (processed by the Engine as Wasm) and the standalone engine. In particular we remove the large if statement that was starting to get unwieldy.

This work is important both because it makes future tests easier to write and because it synchronizes the standalone and wasm engine instances in our tests (this latter point is a prerequisite for properly testing the hashchain).

Some notes about the PR:

  1. I renamed the constant ORIGIN to DEFAULT_AURORA_ACCOUNT_ID because I felt the latter is a more descriptive name for what the constant represents.
  2. The standalone engine is now present in AuroraRunner by default to make out testing more robust (for example tests will now automatically fail if a new state-mutating method is added to the Engine's lib.rs without also being added to the standalone implementation). This means there are a few places were I need to explicitly remove the standalone instance where AuroraRunner is used as something other than an Engine instance (modexp and xcc tests).
  3. Some tests use view calls to inspect the Engine state and make assertions. These calls are not present in the standalone because it only cares about transactions that mutate the state. To make view calls in AuroraRunner it is now required to call .one_shot() before the calls. This essentially tells the testing framework that you are making a view call so no state modifications will be made and therefore we can ignore the standalone.

@birchmd birchmd added C-enhancement Category: New feature or request A-testing Area: If something has added tests, or changed them. rust Pull requests that update Rust code labels Aug 2, 2023
@birchmd birchmd requested a review from aleksuss August 2, 2023 14:59
engine-standalone-storage/src/sync/mod.rs Outdated Show resolved Hide resolved
@birchmd birchmd force-pushed the chore/birchmd/tests-refactor branch from 6f99fa6 to 8b41e8e Compare August 3, 2023 14:16
Copy link
Member

@aleksuss aleksuss left a comment

Choose a reason for hiding this comment

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

LGTM

@birchmd birchmd added this pull request to the merge queue Aug 3, 2023
Merged via the queue into develop with commit 2e1f1a0 Aug 3, 2023
20 checks passed
@birchmd birchmd deleted the chore/birchmd/tests-refactor branch August 3, 2023 15:23
aleksuss pushed a commit that referenced this pull request Aug 10, 2023
#810)

## Description

This is the first in a series of PRs that is meant to split up #705 .
The idea is to merge the changes which are made in that PR in logical
chunks until eventually the whole hashchain implementation is in. Doing
the work in smaller pieces will both make it easier to review and
prevent us from needing to maintain large, long-lived feature branches.

This first PR pulls in the transaction transaction parsing logic from
[borealis-engine-lib](https://github.com/aurora-is-near/borealis-engine-lib)
into this repo (in a future PR we will remove the duplicated code from
borealis-engine-lib). The logic is used here to simplify how
transactions are handled in tests because all transactions can
automatically be passed to both the Near runtime (processed by the
Engine as Wasm) and the standalone engine. In particular we remove the
large `if` statement that was starting to get unwieldy.

This work is important both because it makes future tests easier to
write and because it synchronizes the standalone and wasm engine
instances in our tests (this latter point is a prerequisite for properly
testing the hashchain).

Some notes about the PR:

1. I renamed the constant `ORIGIN` to `DEFAULT_AURORA_ACCOUNT_ID`
because I felt the latter is a more descriptive name for what the
constant represents.
2. The standalone engine is now present in `AuroraRunner` by default to
make out testing more robust (for example tests will now automatically
fail if a new state-mutating method is added to the Engine's `lib.rs`
without also being added to the standalone implementation). This means
there are a few places were I need to explicitly remove the standalone
instance where `AuroraRunner` is used as something other than an Engine
instance (modexp and xcc tests).
3. Some tests use view calls to inspect the Engine state and make
assertions. These calls are not present in the standalone because it
only cares about transactions that mutate the state. To make view calls
in `AuroraRunner` it is now required to call `.one_shot()` before the
calls. This essentially tells the testing framework that you are making
a view call so no state modifications will be made and therefore we can
ignore the standalone.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testing Area: If something has added tests, or changed them. C-enhancement Category: New feature or request rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants