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

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

Merged
merged 2 commits into from
Mar 25, 2024

Conversation

erights
Copy link
Contributor

@erights erights commented Mar 7, 2024

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 erights self-assigned this Mar 7, 2024
@erights erights requested review from dtribble and ivanlei March 7, 2024 01:27
@erights
Copy link
Contributor Author

erights commented Mar 7, 2024

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

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

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
Copy link
Contributor Author

erights commented Mar 11, 2024

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 erights mentioned this pull request Mar 24, 2024
2 tasks
@erights erights requested review from kriskowal, kumavis and rekmarks and removed request for dtribble and ivanlei March 24, 2024 01:58
@erights erights marked this pull request as ready for review March 24, 2024 01:59
@erights erights requested a review from gibson042 March 24, 2024 01:59
@erights
Copy link
Contributor Author

erights commented Mar 24, 2024

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 ?

Copy link
Member

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good place to start. Thanks. I hope @rekmarks and @kumavis bring questions.

@kriskowal
Copy link
Member

The TypeScript errors in CI are opaque to me.

@turadg
Copy link
Member

turadg commented Mar 24, 2024

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 erights enabled auto-merge (squash) March 24, 2024 18:12
@erights erights disabled auto-merge March 24, 2024 18:12
@erights
Copy link
Contributor Author

erights commented Mar 24, 2024

@turadg
Copy link
Member

turadg commented Mar 24, 2024

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.

The 'import by redefine' was causing an error resolving the template parameters.
e.g. M was M_1 in the slot, but with no binding.

I don't know if it was due to bug in TS, but bumping to the latest (5.4.3)
didn't solve it.
@turadg
Copy link
Member

turadg commented Mar 25, 2024

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, 5be642f adds the fix now.

@erights erights merged commit 39b8599 into master Mar 25, 2024
18 checks passed
@erights erights deleted the mark-a-bridge-too-Far branch March 25, 2024 15:55
@dckc
Copy link
Contributor

dckc commented Mar 25, 2024

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:

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

Successfully merging this pull request may close these issues.

4 participants