chore(lib/runtime): CheckRuntimeVersion independent from existing runtime instance

[qdm12]

Changes

Blocked by #2686 and #2685

• CheckRuntimeVersion as function instead of instance method
  • No dependency on existing instance
  • Fix: do not modify parent instance allocator
  • Fix: close temporary instance on exit
  • Remove CheckRuntimeVersion from Instance interface
• ext_misc_runtime_version_version_1 uses CheckRuntimeVersion

Tests

Issues

#2418

Primary Reviewer

@timwu20

[codecov]

Codecov Report

Merging #2687 (6bce0f6) into development (2e57b1e) will decrease coverage by 0.02%.
The diff coverage is 71.42%.

@@               Coverage Diff               @@
##           development    #2687      +/-   ##
===============================================
- Coverage        62.81%   62.79%   -0.03%     
===============================================
  Files              213      213              
  Lines            27069    27057      -12     
===============================================
- Hits             17003    16990      -13     
- Misses            8514     8519       +5     
+ Partials          1552     1548       -4     

[timwu20]

I understand why you removed this function from the interface since it creates a new instance to do the version check, but how will we mock this out now? We'd have to change the function signatures of all callers of this function to include a func([]byte) (Version, error)

[qdm12]

I'd say it's better to not mock it, since:

• it's fast
• it's all CPU/in-memory and no IO
• it's predictable
• it adds testing depth which we should crave for when those 3 above are true

If you still want to mock it somewhere though, then you can set it as a functional field on the receiving struct like

type Service struct {
  checkRuntimeVersion(code []byte) (version Version, err error)
}

func New() *Service {
  return &Service{
    checkRuntimeVersion: wasmer.CheckRuntimeVersion,
  }
}

[timwu20]

I don't think it's fast at all. It's instantiating a wasmer instance, loading the code in, binding the imports, etc. Now all of the unit tests are runtime specific if we remove it from the Instance interface.

Why aren't we just implementing a RuntimeVersion() (Version, error) (no blob) on Instance. Then anyone who needs to version check needs to instantiate an instance via NewInstance.

[qdm12]

It takes 500ms to load the version, so not that fast indeed.
But if you have a look, there was no configuration of the CheckRuntimeVersion mock method expectation, so it won't change a thing with our code as it is.
And as I mentioned above, if in the future we want to mock the runtime check (for test coverage or testing speed), we can just set it as a field on the receiving struct (like we would with timeNow func() time.Time to mock time).

Also (in my opinionated opinion), having a function as a method just for the sake of it being part of an interface for testing is a bit weird, especially if it doesn't depend on the struct/implementation at all (wasn't the case before though, but it is now).

[timwu20]

I looked at the interface again. The Instance interface includes a Version() (Version, error) method. I think we should remove CheckRuntimeVersion from the runtime package and make this a private method in dot/state and a helper function for the tests. What do you think?

[qdm12]

Good point, although that would require to export a bunch of things such as setupVM, the wasmer Instance struct fields vm and ctx, and I think ultimately we should keep this in the wasmer package since it's quite tightly coupled with wasmer thingies? I would rather export CheckRuntimeVersion than these other function/fields, what do you think?

[timwu20]

why do you need to export those private functions? To check a version of a given wasm blob, you instantiate an instance by calling NewInstance(code []byte, cfg runtime.InstanceConfig) and call Version() on that instance.

[qdm12]

Good point, sorry I misunderstood.
I changed CheckRuntimeVersion to use NewInstance, simplifying it a bit / reducing code duplication (although it sets up extra stuff it doesn't need, but that's a dep injection problem for another day).

On the other hand, ext_misc_runtime_version_version_1 in wasmer/imports.go uses CheckRuntimeVersion so I think we should still keep it in the wasmer package? 🤔

[timwu20]

I have dissonance with having a function on the public package API that achieves the same functionality that can be achieved by instantiating an instance and calling a receiver method on that instance. But that would lead to code duplication cause we use it in various places. I gave it a shot in this commit, and I'm not really happy with it. So I'm ok with leaving with leaving it in the wasmer package.

[timwu20]

I have dissonance with having a function on the public package API that achieves the same functionality that can be achieved by instantiating an instance and calling a receiver method on that instance. But that would lead to code duplication cause we use it in various places. I gave it a shot in this commit, and I'm not really happy with it. So I'm ok with leaving with leaving it in the wasmer package.

[noot]

like @timwu20 mentioned there isn't really a need for this function to remain in the runtime/wasmer package anymore, might make more sense for it to be in dot/state

[qdm12]

Kept it there to avoid replicating 3 times (lib/runtime/wasmer, dot/state, and somewhere else too) as per what Tim described.

Also it might get a bit more complex due to Core_version being deprecated, and we won't need to create an instance anymore for newer runtimes, so we should definitely have it in one place. (see paritytech/substrate#8688)

[qdm12]

I have dissonance with having a function on the public package API that achieves the same functionality that can be achieved by instantiating an instance and calling a receiver method on that instance.

If it's stateless, I would say it's fine to have as a 'pure' function (without interfacing etc.). The caller can always have it as a function struct field (or other).

But that would lead to code duplication cause we use it in various places. I gave it a shot in this commit, and I'm not really happy with it. So I'm ok with leaving with leaving it in the wasmer package.
there isn't really a need for this function to remain in the runtime/wasmer package anymore, might make more sense for it to be in dot/state

Yeah I would rather have it in the wasmer package as a helper function than duplicated in 3 places.

[github-actions]

🎉 This PR is included in version 0.7.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀