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

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

Open
anorth opened this issue Feb 27, 2023 · 2 comments
Open

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

anorth opened this issue Feb 27, 2023 · 2 comments

Comments

@anorth
Copy link
Member

anorth commented Feb 27, 2023

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
Copy link
Member

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 #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
Copy link
Member Author

anorth commented Feb 28, 2023

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

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

No branches or pull requests

2 participants