stellar / rs-soroban-env

Rust environment for Soroban contracts.
Apache License 2.0
60 stars 40 forks source link

Enable VM execution in a WASM environment by guarding time track behind `time` feature #1315

Closed heytdep closed 8 months ago

heytdep commented 8 months ago

What

This PR guards the usage of Instant behind a time feature, at least for a VM invocation.

Why

This enables the soroban vm to execute within a WASM context. For instance, without this modification running the soroban VM in wrangler resulted in

✘ [ERROR] panicked at null.<anonymous> (library/std/src/sys/wasm/../unsupported/time.rs:13:9):

  time not implemented on this platform

  Stack:

  Error
      at nt
  (file:///mnt/storagehdd/sdf-work/soroflare/soroflare-wrangler/build/worker/shim.mjs:2:4496)
      at [object Object]x19354a
      at [object Object]x163c0d
      at [object Object]x1730b8
      at [object Object]x2e74d
      at [object Object]x3f76d
      at [object Object]x1dc90
      at [object Object]x8de53
      at [object Object]x2cee6
      at [object Object]x129a8d

✘ [ERROR] Uncaught (in response) Error: The script will never generate a response.

Discord reference: https://discord.com/channels/897514728459468821/1187049026831523940/1187457822930260018

Known limitations

N/A

Implementation details

I'm using a closure here but a named fn is probably better.

heytdep commented 8 months ago

I'm also pretty sure that Budget::get_time() would require some changes for consistency (ex if "time" then error) but I don't want to mess around with this without having a solid understanding of the whole codebase, just leaving this PR here as a starting point.

jayz22 commented 8 months ago

Thanks for getting started with the fix. I think for now it is okay to proceed with the minimally disruptive fix to unblock your use case. So I wouldn't worry too much about things like returning different errors on feature on vs off, or gating the budget behind it. The time measurement is a one-off case to get better metrics for Vm instantiation (for all other types we currently return 0). It will likely go through more changes in the next protocol release, when we improve Vm instantiation cost model. We might either expand it to include other cost types (which will require a more holistic design), or remove it all together.

I'm using a closure here but a named fn is probably better.

I prefer a named function for maintainability.

heytdep commented 8 months ago

@jayz22 moved the closure to a vm instantiate method. Following your thoughts I haven't guarded the budget's get_time and track_time functionalities (and the struct field) behind the feature.