search

filecoin-project / actors-utils

Collection of libraries to implement common patterns and standards on the Filecoin Virtual Machine
Other
15 stars 13 forks source link

Add a test to simulate integration into built-in actors #202

Open anorth opened 1 year ago

anorth commented 1 year ago

The built-in actors are the first and currently only consumer of these libraries (though of course we don't expect that to continue). Ease of integration there is important. However that environment has it's own imperfect design choices, e.g. the Runtime trait that abstracts over the FVM syscalls and presents a mockable interface for tests.

We could add some test code that starts with a similar Runtime interface, then adapts the token library (ActorRuntime) to it and uses it for some basic actor operation. Then we can see the static impact of API changes immediately, without going to the effort of a trial integration into built-in actors every time we want to improve something, and avoiding potential impedance mismatch when we later integrate.

FYI @Stebalien @arajasek do you think this could help?

Stebalien commented 1 year ago

I'm not too concerned with how changes here interact with the builtin actors runtime, I'm more concerned with how changes to this library might impact downstream users.

Specifically, I want to make sure that improvements to the abstractions in this library actually improve the usability of these abstractions in practice. Toy examples in this library will help, but the only sure-fire way to validate usability is to try out changes in the builtin-actors repo (for the moment, at least).

For example, the change where Token::wrap started taking the ActorRuntime by reference instead of by-value snuck into https://github.com/helix-onchain/filecoin/pull/190 but ended up causing a somewhat painful migration downstream because the ActorRuntime now needed a place to live on the stack.

Basically, I'm asking for something exploratory. Before merging a PR with significant API changes:

  1. Checkout a copy of the builtin actors.
  2. Patch in the change to this library.
  3. Try fixing any compilation errors.
anorth commented 1 year ago

I think we're saying a similar thing? The built-in actors are a downstream user. My proposal aims to detect something like that ActorRuntime-needs-the-stack issue earlier by having it emerge in code in this repo, which is much quicker to discover and so a better design environment. I'm not saying we should never also try an actual integration, but we can make it less part of tight dev cycle.s