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

Sandbox Runtime #1

Merged
merged 26 commits into from
Sep 2, 2021
Merged

Sandbox Runtime #1

merged 26 commits into from
Sep 2, 2021

Conversation

ChaoticTempest
Copy link
Member

@ChaoticTempest ChaoticTempest commented Aug 20, 2021

Rough implementation of SandboxRuntime: not everything is in place, but works for the most part in testing things locally. Also, to make it more idiomatic to rust's runtime libraries, there's no concept of Runner implemented, since Runtime is basically the runner.

  • After this PR, will rename the repo to runner-rs.
  • All the helper functions got moved to runner/rpc/api.rs, but will likely be changed over to use NAR after this PR gets merged.
  • SandboxRuntime has been added, but mainnet and testnet runtimes are not implemented yet (should be easy in a future ticket to implement and hopefully not "famous last words").

Sample usage:

use runner::*;

#[runner::test(sandbox)]
async fn test_deploy() {
    let (contract_id, signer) = dev_deploy(Path::new("path/to/file.wasm"))
        .await
        .expect("could not dev-deploy contract");

    let result = view(
        contract_id,
        "function_name".to_string(),
        r#""some_arg": "some_value""#.into(),
    ).await.expect("could not call into view function");

    assert_eq!(result, OUR_EXPECTED_RESULT);
}

or using it within a binary directly like so:

use runner::*;

#[runner::main(sandbox)]
async fn main() {
}

The hope is we can have #[runner::test(testnet)] and #[runner::test(mainnet)] options too, which do work, but just the runtime hasn't been implemented for those.

Let me know if there's anything anybody doesn't understand. The design for the runtime was mainly taken from tokio itself.

Copy link
Contributor

@austinabell austinabell left a comment

Choose a reason for hiding this comment

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

tests don't work locally because we have diff sandbox binary locations. I'll dig more into the internals when I can test things by running

runner-macros/src/entry.rs Show resolved Hide resolved
runner-macros/src/entry.rs Outdated Show resolved Hide resolved
runner-macros/src/entry.rs Show resolved Hide resolved

pub(crate) fn enter(flavor: RuntimeFlavor) -> EnterGuard {
RT_CONTEXT.with(|ctx| {
let old = ctx.borrow_mut().replace(flavor);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little confused by this, because if used asynchronously wouldn't this cause race conditions where another thread/task would access the modified environment rather than the intended one?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was also another thing I took from tokio. This is behind a thread_local so using it asynchronously shouldn't be an issue. You can also take a look at how tokio uses it context::enter

There is another case where the runtime is multi-threaded, but the runtime will do the thread spawning directly instead of the developer. During thread spawning we also call enter for that specific thread to have this runtime context as well. But the current rt implementation is only single threaded.

runner/src/runtime/local.rs Outdated Show resolved Hide resolved
runner/src/rpc/api.rs Outdated Show resolved Hide resolved
runner/src/runtime/local.rs Outdated Show resolved Hide resolved
runner/tests/deploy.rs Outdated Show resolved Hide resolved
runner/tests/deploy.rs Show resolved Hide resolved
runner/src/runtime/local.rs Outdated Show resolved Hide resolved
@ChaoticTempest
Copy link
Member Author

tests don't work locally because we have diff sandbox binary locations. I'll dig more into the internals when I can test things by running

@austinabell Ahh, yea sorry about that. Just hardcoded it for now (base on my binary location) so there's proper cleanup after the test finishes. We just need to have a cargo crate of the sandbox binary or a utility that can easily fetch the sandbox location if we install it thru npm.

examples/src/nft.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@austinabell austinabell left a comment

Choose a reason for hiding this comment

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

Seems like a decent starting point, as long as the install and binary usage is updated, sleeps are updated to not be so rigid, and certain interfaces are cleaned up in another PR.

Definitely want to make this accessible for anyone to use without assuming a specific binary location of something installed externally to test this

@@ -97,7 +95,7 @@ pub async fn view(
method_name: String,
args: FunctionArgs,
) -> Result<serde_json::Value, String> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we swap this to be generic on serde deserialize to not have to go through serde_json::Value if a caller doesn't want? (prob in another PR)

Copy link
Member Author

Choose a reason for hiding this comment

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

yea definitely. A lot of the RPC calls need to be re-evaluated too. I'll make a ticket for another PR later once this merges

runner/src/runtime/local.rs Outdated Show resolved Hide resolved
runner/src/runtime/local.rs Outdated Show resolved Hide resolved
Copy link

@willemneal willemneal left a comment

Choose a reason for hiding this comment

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

Approved, just have one comment about the type name flavor over network or even endpoint.

@ChaoticTempest ChaoticTempest merged commit 6c9a962 into main Sep 2, 2021
@ChaoticTempest ChaoticTempest mentioned this pull request Sep 3, 2021
1 task
@ChaoticTempest ChaoticTempest deleted the sandbox-runner branch December 2, 2021 01:05
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.

3 participants