refactor(cli,daemon): start replacing Far with makeExo

erights commented 6 months ago

closes: #XXXX refs: #XXXX

Description

To teach people how to write upgradable code, we should stop teaching Far and start teaching use of exo-making operations. This first step makes the easiest such transition, with the following mostly automated steps (that should also apply to agoric-sdk):

replace: Far\('(.*)', \{ with: makeExo('$1', M.interface('$1', {}, { defaultGuards: 'passable' }), {
doing yarn format

then putting

import { makeExo } from '@endo/exo';
import { M } from '@endo/patterns';

where necessary,

removing Far import when no longer used,
updating package.json to depend on "@endo/exo" and "@endo/patterns"

makeExo is implicitly heap-only, whereas we should really transition to the appropriate zones, including the heap zone. But the zone package has not yet been moved from agoric-sdk to endo, so this is not yet possible here. In any case, it can follow as a later step.

The use of defaultGuards: is how we skip needing to write the method guards in this first step. This enables incremental progress from there of writing accurate method guards.

Security Considerations

If this PR rewrote these to defaultGuards: 'raw', then this rewrite would be security equivalent to the original.

defaultGuards: 'passable' does enforce the minimal requirement we need for separation at exo boundaries, which is that only passable values get directly passed in or out. However, this doesn't actually make the exo boundary a defensive boundary until we complete three tasks

Use exos rather that Far. Hence this PR.
Stop promises from leaking unchecked fulfillment across the call barrier.
- Use M.await(pattern) (with M.callWhen) for promises that should or could be awaited before the raw method is called. This checks the fulfillment against the argument pattern.
- Shift to Vows for promises that cannot be M.awaited there.
Ensure any error thrown from an exo call gets rethrown as a Passable error as make by toPassableError. Ensure anything else thrown is rethrown as something that it at least Passable, and likely also required to be pure data.

So, again, this PR is just a step towards those goals.

As the interface guards are incrementally refined with proper method guards, we get a huge increase in defensive input validation. The underlying raw method will never see arguments that did not pass those method guards. The raw method code must still do any input validation of such guard-passing arguments, but this is a much smaller burden.

This is why our rewrite puts a per-makeExo interface inline, rather than just referring to a shared one or even saying undefined: To encourage the incremental expansion of these with accurate method guards.

Once all methods have explicit method guards, the defaultGuards: option should be removed.

Scaling Considerations

All this extra checking that arguments and return results are Passable is extra overhead that may be significant. Likely bottlenecking on passStyleOf that we already know is a painful hotspot. We'll only know how bad this is by measuring.

Documentation Considerations

This is largely the point of this PR, and related upcoming agoric-sdk PRs. People learn to program for our platform by looking at example code. We want to shift from teaching Far to teaching exo making patterns to smooth their transition to writing upgradable code.

Testing Considerations

All dynamic CI checks pass with this change, which is a bit of a surprise. Whether it is a pleasant or unpleasant surprise depends on whether this indicates the transition has no problems, or just that the CI tests aren't catching those problems. The likely problems are do to using defaultGuards: 'passable' rather than defaultGuards: 'raw'. It was perfectly correct to call Far objects with non-passable arguments. If such uses were covered by CI, we should have gotten a visible failure.

Currently, CI is giving a static error

$ tsc --build tsconfig.build.json
Error: ../exo/src/exo-makers.d.ts(5,44): error TS2304: Cannot find name 'S'.
Error: ../exo/src/exo-makers.d.ts(5,47): error TS2304: Cannot find name 'M_1'.
Error: ../exo/src/exo-makers.d.ts(9,45): error TS2304: Cannot find name 'S'.
Error: ../exo/src/exo-makers.d.ts(9,48): error TS2304: Cannot find name 'F_1'.
Error: ../exo/src/exo-makers.d.ts(10,261): error TS2304: Cannot find name 'S'.
Error: ../exo/src/exo-makers.d.ts(10,264): error TS2304: Cannot find name 'M_1'.

I don't understand this error and will need help figuring out what to do about it.

Compatibility Considerations

By using defaultGuards: 'passable' rather than defaultGuards: 'raw', we do have a chance of breaking caller that were correct.

Upgrade Considerations

Since neither Far nor makeExo make durable things, this first step has no effect on upgrade. Further, the abstractions changed by this PR are unlikely to become durable in the future. However, a next step should still change some of these to use a zone defaulting to the heapZone. To the extent that the abstraction-making code can be parameterized by zone, then it can become up to the caller whether to use the heap zone or not.

[ ] Includes *BREAKING*: in the commit message with migration instructions for any breaking change.
[ ] Updates NEWS.md for user-facing changes.

erights commented 6 months ago

@dtribble and @ivanlei , this is from our conversation today, so I added you as reviewers. Please look and then we should figure out how to proceed from there.

kriskowal commented 6 months ago

Attn @rekmarks @kumavis @danfinlay since this impacts the daemon and CLI, which we’re maintaining together going forward. I’m not sure what role Far will have in evaluate-in-worker-and-compartment formulas, except as an expedient for CLI eval usage maybe?

kriskowal commented 6 months ago

And, I would like one of us to pick this up and run with it, consulting @erights so we can discover socialize the migration experience.

erights commented 6 months ago

And, I would like one of us to pick this up and run with it, consulting @erights so we can discover socialize the migration experience.

Thanks. Staying in Draft in any case until we have evidence that the performance impact is not too bad.

But see "Security Considerations" for why it is important. Though still not urgent.

Update

Above I say

Staying in Draft in any case until we have evidence that the performance impact is not too bad.

Since this PR makes this change only to @endo/cli and @endo/daemon, I don't think this concern should block merging. Reviewers, please let me know if you disagree.

erights commented 5 months ago

This is now R4R

@dtribble @ivanlei , I removed you as reviewers.

@kriskowal @kumavis @rekmarks @gibson042 I added you as the reviewers.

I'm still getting the static CI error mentioned under "Testing Considerations" in the PR comment. It is something about TypeScript generated files that I cannot figure out. Need help. Attn @turadg @michaelfig ?

kriskowal commented 5 months ago

The TypeScript errors in CI are opaque to me.

turadg commented 5 months ago

I'm still getting the static CI error mentioned under "Testing Considerations" in the PR comment. It is something about TypeScript generated files that I cannot figure out. Need help. Attn @turadg @michaelfig ?

Something with the templated type imports. 96a15b5

erights commented 5 months ago

Hi @turadg, thanks! But still getting the symptom at https://github.com/endojs/endo/actions/runs/8411192334/job/23030512852?pr=2118#step:9:76

turadg commented 5 months ago

Hi @turadg, thanks! But still getting the symptom at https://github.com/endojs/endo/actions/runs/8411192334/job/23030512852?pr=2118#step:9:76

Looks like you force pushed over the commit with the fix and it's gone. I'll try to retrive it when I'm back at my work computer.

turadg commented 5 months ago

Looks like you force pushed over the commit with the fix and it's gone.

When I did git pull rebase=interactive it thought the commit was part of the history, so my guess it got lost in a merge conflict resolution. Not sure why.

Anyway, 5be642f1d8bad6027a2c4cd0a38f8002358cc970 adds the fix now.

dckc commented 5 months ago

Scaling Considerations

All this extra checking that arguments and return results are Passable is extra overhead that may be significant. Likely bottlenecking on passStyleOf that we already know is a painful hotspot.

to wit:

https://github.com/Agoric/agoric-sdk/issues/8862

endojs / endo