Skip to content

Commit

Permalink
fix(async-flow): fix endowment equate bug (#9736)
Browse files Browse the repository at this point in the history
closes: #9830
refs: #9722 #9719

## Description

#9719 originally failed on upgrade replay for an endowment. It revealed a bug introduced to async-flow when adding support for endowments. Because of the so-called "unwrapping" of some guests, there can be two guests corresponding to one host, with the host of course only mapping back to one of them -- the outer one. This makes `bijection.js` more complicated and irregular than an actual bijection.

`equate(g, h)` had a test for early return, if the `g` and `h` were already "equated", i.e., were corresponding guest and host. But the equate test was written before the elaboration of bijection. In fact, it should only test whether this guest `g` maps to the host `h`, irrespective of whether `h` maps back to this `g`.

Additional testing revealed that the unwrapped function was also not passable, and would fail to be passed back as an argument.

### Security Considerations
None

### Scaling Considerations
None

### Documentation Considerations
This change potentially makes the diagnostic error when misusing async-flow slightly less precise.

### Testing Considerations
Introduces equate checks in the endowments test exercising the bijection. For endowments with are further "unwrapped", we test both the original guest (which was previously failing) and the unwrapped one (which also was, but for a different reason).

Since #9719 landed with a failing test, this PR also sets that test as passing, effectively working as an integration test of functions as endowments.

### Upgrade Considerations
Can be deployed as a new version of the async-flow NPM package.
  • Loading branch information
mergify[bot] authored Oct 4, 2024
2 parents 2670a4e + 0b38035 commit 4bc297d
Show file tree
Hide file tree
Showing 5 changed files with 51 additions and 21 deletions.
10 changes: 7 additions & 3 deletions packages/async-flow/src/endowments.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
import { Fail } from '@endo/errors';
import { E } from '@endo/eventual-send';
import { isPromise } from '@endo/promise-kit';
import { isRemotable, isPassable, GET_METHOD_NAMES } from '@endo/pass-style';
import {
isRemotable,
isPassable,
GET_METHOD_NAMES,
Far,
} from '@endo/pass-style';
import { M, objectMap } from '@endo/patterns';
import { prepareVowTools, toPassableCap } from '@agoric/vow';
import { isVow } from '@agoric/vow/src/vow-utils.js';
Expand Down Expand Up @@ -63,8 +68,7 @@ export const prepareEndowmentTools = (outerZone, outerOptions = {}) => {

const functionUnwrapper = outerZone.exo('FunctionUnwrapper', UnwrapperI, {
unwrap(guestWrapped) {
const unwrapped = (...args) => guestWrapped.apply(args);
return harden(unwrapped);
return Far('UnwrappedFunction', (...args) => guestWrapped.apply(args));
},
});

Expand Down
24 changes: 16 additions & 8 deletions packages/async-flow/src/equate.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,14 @@ export const makeEquate = bijection => {
Fail`unequal ${g} vs ${h}`;
return;
}
if (bijection.has(g, h)) {
if (bijection.hasGuest(g) && bijection.guestToHost(g) === h) {
// Note that this can be true even when
// `bijection.hostToGuest(h) !== g`
// but only when the two guests represent the same host, as
// happens with unwrapping. That why we do this one-way test
// rather than the two way `bijection.has` test.
// Even in this one-way case, we have still satisfied
// the equate, so return.
return;
}
const gPassStyle = passStyleOf(g);
Expand All @@ -41,7 +48,8 @@ export const makeEquate = bijection => {
// TODO when we do, delete the `throw Fail` line and uncomment
// the two lines below it.
// We *do* support passing a guest wrapper of a hostVow back
// to the host, but that would be cause by `bijection.has` above.
// to the host, but that would be caught by `bijection.guestToHost`
// test above.
throw Fail`guest promises not yet passable`;
// `init` does not yet do enough checking anyway. For this case,
// we should ensure that h is a host wrapper of a guest promise,
Expand Down Expand Up @@ -92,10 +100,10 @@ export const makeEquate = bijection => {
case 'remotable': {
// Note that we can send a guest wrapping of a host remotable
// back to the host,
// but that should have already been taken care of by the
// `bijection.has` above.
// but that should have already been caught by the
// `bijection.guestToHost` above.
throw Fail`cannot yet send guest remotables to host ${g} vs ${h}`;
// `init` does not yet do enough checking anyway. For this case,
// `unwrapInit` does not yet do enough checking anyway. For this case,
// we should ensure that h is a host wrapper of a guest remotable,
// which is a wrapping we don't yet support.
// bijection.unwrapInit(g, h);
Expand All @@ -104,10 +112,10 @@ export const makeEquate = bijection => {
case 'promise': {
// Note that we can send a guest wrapping of a host promise
// (or vow) back to the host,
// but that should have already been taken care of by the
// `bijection.has` above.
// but that should have already been caught by the
// `bijection.guestToHost` above.
throw Fail`cannot yet send guest promises to host ${g} vs ${h}`;
// `init` does not yet do enough checking anyway. For this case,
// `unwrapInit` does not yet do enough checking anyway. For this case,
// we should ensure that h is a host wrapper of a guest promise,
// which is a wrapping we don't yet support.
// bijection.unwrapInit(g, h);
Expand Down
24 changes: 22 additions & 2 deletions packages/async-flow/test/endowments.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { makeDurableZone } from '@agoric/zone/durable.js';
import { forwardingMethods, prepareEndowmentTools } from '../src/endowments.js';
import { makeConvertKit } from '../src/convert.js';
import { prepareBijection } from '../src/bijection.js';
import { makeEquate } from '../src/equate.js';

const { ownKeys } = Reflect;

Expand Down Expand Up @@ -84,7 +85,16 @@ const testEndowmentPlay = async (t, zone, gen, isDurable) => {
t.is(endowment.state[`${gen}_foo`], `${gen} foo`);
t.is(wrapped.state.get(`${gen}_foo`), `${gen} foo`);

const makeBijection = prepareBijection(zone, unwrap);
const guestWrappers = new Map();
const unwrapSpy = (hostWrapped, guestWrapped) => {
const unwrapped = unwrap(hostWrapped, guestWrapped);
if (unwrapped !== guestWrapped) {
guestWrappers.set(hostWrapped, guestWrapped);
}
return unwrapped;
};

const makeBijection = prepareBijection(zone, unwrapSpy);
const bij = zone.makeOnce('bij', makeBijection);

const makeGuestForHostRemotable = hRem => {
Expand Down Expand Up @@ -114,13 +124,23 @@ const testEndowmentPlay = async (t, zone, gen, isDurable) => {
t.is(unwrapped.storable.exo.name(), `${gen} exo`);
t.is(passStyleOf(unwrapped.far), 'remotable');
t.is(unwrapped.far.name(), `${gen} far`);
t.false(isPassable(unwrapped.function));
t.is(passStyleOf(unwrapped.function), 'remotable');
t.is(typeof unwrapped.function, 'function');
t.is(unwrapped.function(), `${gen} function`);
t.is(unwrapped.array[0](), `${gen} f1`);
t.false(isPassable(unwrapped.state));
t.is(typeof unwrapped.state, 'object');
t.is(unwrapped.state[`${gen}_foo`], `${gen} foo`);

const equate = makeEquate(bij);

const { state: _1, ...passableUnwrapped } = unwrapped;
const { state: _2, ...passableWrapped } = wrapped;

t.notThrows(() => equate(harden(passableUnwrapped), harden(passableWrapped)));
for (const [hostWrapped, guestWrapped] of guestWrappers) {
t.notThrows(() => equate(guestWrapped, hostWrapped));
}
};

const testEndowmentBadReplay = async (_t, _zone, _gen, _isDurable) => {
Expand Down
10 changes: 5 additions & 5 deletions packages/async-flow/test/equate.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,24 +57,24 @@ const testEquate = (t, zone, showOnConsole = false) => {
bij.unwrapInit(g1, h1);
t.notThrows(() => equate(g1, h1));
t.throws(() => equate(g1, h2), {
message: 'internal: g->h "[Alleged: g1]" -> "[Vow]" vs "[Alleged: h1]"',
message: 'unequal passStyles "remotable" vs "tagged"',
});
t.throws(() => equate(g2, h1), {
message: 'internal: unexpected h->g "[Alleged: h1]" -> "[Alleged: g1]"',
message: 'unequal passStyles "promise" vs "remotable"',
});
bij.unwrapInit(g2, h2);
equate(g2, h2);

t.throws(() => equate(g1, h2), {
message: 'internal: g->h "[Alleged: g1]" -> "[Vow]" vs "[Alleged: h1]"',
message: 'unequal passStyles "remotable" vs "tagged"',
});
t.throws(() => equate(g2, h1), {
message: 'internal: g->h "[Promise]" -> "[Alleged: h1]" vs "[Vow]"',
message: 'unequal passStyles "promise" vs "remotable"',
});

equate(harden([g1, g2]), harden([h1, h2]));
t.throws(() => equate(harden([g1, g2]), harden([h1, h1])), {
message: '[1]: internal: g->h "[Promise]" -> "[Alleged: h1]" vs "[Vow]"',
message: '[1]: unequal passStyles "promise" vs "remotable"',
});

const gErr1 = harden(makeError(X`error ${'redacted message'}`, URIError));
Expand Down
4 changes: 1 addition & 3 deletions packages/boot/test/orchestration/contract-upgrade.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ test.after.always(t => t.context.shutdown?.());
* upgrades, and the ability to resume an async-flow for which a host vow
* settles after an upgrade.)
*/
test.failing('resume', async t => {
test('resume', async t => {
const { walletFactoryDriver, buildProposal, evalProposal, storage } =
t.context;

Expand Down Expand Up @@ -73,8 +73,6 @@ test.failing('resume', async t => {
buildProposal('@agoric/builders/scripts/testing/fix-buggy-sendAnywhere.js'),
);

// FIXME https://github.com/Agoric/agoric-sdk/issues/9303
// This doesn't yet get past 'sending'
t.deepEqual(getLogged(), [
'sending {0} from cosmoshub to cosmos1whatever',
'got info for denoms: ibc/toyatom, ibc/toyusdc, ubld, uist',
Expand Down

0 comments on commit 4bc297d

Please sign in to comment.