contract upgrade proposals fragile due to promiseSpace reset strange behavior

[dckc]

Describe the bug

while working on upgrade testing (#6099) we ran into strange behavior that causes subtle failures in core eval proposals.

it seems to come down to a bug around the reset() method from makePromiseSpace.

To Reproduce

Steps to reproduce the behavior:

test('resolve after reset', async t => {
  /** @type {MapStore<string, string>} */
  const store = makeScalarBigMapStore('stuff', { durable: true });

  const { consume, produce } = makePromiseSpace({ store });
  {
    const { resolve, reset } = produce.foo;
    // const foo2 = consume.foo;
    reset();
    resolve(1);
    // t.is(await foo2, 1);
    t.is(await consume.foo, 1);
  }

  {
    const { resolve, reset } = produce.foo;
    reset();
    resolve(2);
    t.is(await consume.foo, 2);
  }
});

Expected behavior

test above ^ passes. Perhaps some other design constraints need some thought.

Platform Environment

• what OS are you using? what version of Node.js?
• is there anything special/unusual about your platform?
• what version of the Agoric-SDK are you using? (run git describe --tags --always)

Additional context

Add any other context about the problem here.

Screenshots

If applicable, add screenshots to help explain your problem, especially for UI interactions.

cc @michaelfig @turadg @warner

[warner]

The basic problem is that reset() isn't changing the closed-over state or pk variables. It deletes name from nameToState, so the next time someone reaches through produce or consume (hitting the Proxy's get trap) and calls provideState, they'll get a new value, but if someone has already captured the resolve or reject (i.e. const { name } = produce for a later name.resolve()), they'll have a resolver for the old promise, which isn't what a new const { name } = consume getter will get.

This is triggered by a pattern in @dckc 's init-proposal test, which wasn't in the unit tests:

const { name } = produce;
name.reset();
name.resolve(newThing);

If the .reset and the .resolve happened in two separate functions, then the second produce.name lookup would have triggered a new provideState(), which would have used the right pk.resolve.

To fix this best, I think we should remove the closed-over variables state and pk, and have all the methods work solely with nameToState.get(name) values. But I need to sketch that out and see if it actually makes sense.

[erights]

Is the abstraction buggy in a way revealed by the test? Or is the test buggy in that it assumes a stronger property of the abstraction than the abstraction is supposed to provide?

[warner]

I think it's the former. It should be legal to capture both reset() and resolve() at the same time, then call reset() followed by resolve(), if only because that's the simplest thing for a "replace the old value with a new one" behavior function to do. Asking these functions to capture the two values separately would preclude the use of a destructuring assignment like { produce: { name: { reset, resolve } } }, which is how most of our existing behavior functions work (albeit none of them use reset, only this special test).