using representatives in stores during init()

[warner]

I think we need to do valToSlotTable.set(tempInstance, instanceKey) before calling tempInstance.init. The user-supplied init function is likely to interact with other representatives and WeakStores, and it might pass itself to one of those for use as a key: imagine the Purse/Payment/DepositFacet, and a case where the deposit facet does a depositLedger.init(deposit, purse) during initialization. We need to get our instanceKey associated with this instance early enough to allow those calls to work.

It's not trivial, though, because those same calls might also read from a WeakStore, and we won't be prepared to build a new Representative until we've saved the data from init. I think we can land this before solving the problem, but we absolutely must build a unit test that exercises it so we know what more needs to be done.

So, actually let's put a comment here reminding us to add that valToSlot addition, but let's not implement it yet, because I think doing it halfway might mask the kind of bug I'm anticipating.

Originally posted by @warner in #1907 (comment)

[warner]

So the current #455 "virtual objects" API we're landing (in #1907) will let users write code like this:

const paymentLedger = makeWeakStore(); // value: balance

  function representPurse(state) {
    const purse = {
      initialize(initialBalance = 0) {
        state.balance = initialBalance;
        state.deposit = makeDeposit(purse);
      },
      deposit(srcPayment) {
        const srcPaymentBalance = paymentLedger.get(srcPayment);
        paymentLedger.delete(srcPayment);
        state.balance += srcPaymentBalance;
        return srcPaymentBalance;
      },
      // ... other methods
    };
    return purse;
  }

  const makePurse = makeKind(representPurse);

  function representPayment() {
    const payment = {
      initialize(amount) {
        paymentLedger.init(payment, amount);
      },
      getAllegedBrand: () => brand,
    };
    return payment;
  }

  const makePayment = makeKind(representPayment);

In Payment's initialize function, it wants to use itself as a key in the paymentLedger to remember its .amount. (Purses remember their amounts directly, as accessor-mediated properties on their state object, but Payments need to be visible to Purse.deposit, so we need a rights-amplification pattern).

This only works if the weak-store code (embedded in makeWeakStore and accessed through paymentLedger) can figure out what the payment argument is a representative of. It can do that after initialize is complete, because makeKind code will add the payment object to the valToSlot table, which is where weak-store looks.

But in the current PR, it doesn't add the entry to valToSlot until after initialize returns, which means this Payment.initialize couldn't work: the paymentLedger.init(payment, amount) call would think payment is a regular object (not a Representative), and the amount data would disappear as soon as this particular Representative went away (which would be a pretty subtle bug: it would work fine within the current delivery, but a subsequent delivery that got a different Representative would fail, because get throws if the key is missing).

The shallow fix is for makeKind to add the payment object to valToSlot before calling payment.initialize. But this opens up a different problem: we aren't really prepared to honor a makeRepresentative call yet. payment.initialize can call whatever arbitrary code it wants, and it can pass itself (payment) to that code, which doesn't know that we're still in the middle of initialization. In particular, we haven't finished performing any state.foo= data assignments yet.

This example is pretty contrived, but I think it demonstrates the problem:

const ledger = makeWeakStore();

const other = {
  subscribeMe(them) {
    ledger.get(them).getAmount2();
  },
};

function representFoo(state) {
  const foo = {
    initialize(amount1, amount2) {
      ledger.init(foo, amount1);
      state.amount2 = amount2;
      other.subscribeMe(foo);
    },
    getAmount2() {
      return state.amount2;
    },
  };
  return harden(foo);
}
const fooMaker = makeKind(representFoo);

For ledger.init to work, we must change makeKind to add foo to valToSlot early, before it calls foo.initialize(). But then when subscribeMe is called, we haven't finished storing amount2 into the LRU cache (we haven't even looked at what initialize did to the temporary state object we gave it). So when subscribeMe calls ledger.get(foo), makeRepresentative will be asked to create a new representative, and it won't have any state with which to populate it, so getAmount2 can't possibly work.

Worse, this pattern looks like it should work, because if state and foo were normal objects (so there was exactly one of them, rather than a new pair for each representative), then the programmer did the right thing: they set all their state (both ledger.init and state.amount2) before revealing themselves to anyone else.

If we were strictly using the rights-amplification pattern everywhere (even for amount2 which doesn't need to be accessed externally), then this wouldn't be a problem. The initialization method would look like:

const fooLedgerAmount1 = makeWeakStore();
const fooLedgerAmount2 = makeWeakStore();
function representFoo() {
  const foo = {
    initialize(amount1, amount2) {
      fooLedger1.init(foo, amount1);
      fooLedger2.init(foo, amount2);
      other.subscribeMe(foo);
   },

and I think adding foo to valToSlot before calling initialize would be sufficient to allow whatever makeRepresentative() calls to work. (The real test is whether there is any code after the call to foo.initialize: if not, the representative is ready to go).

[warner]

cc @FUDCo @erights @dtribble @michaelfig

[FUDCo]

If we enter the new virtual object in the valToSlot table before calling initialize (and we probably need a little bit of extra prep work too, to make sure the object is completely well formed before anybody gets to see it), I'm pretty sure this is not a problem. All that is required for a virtual object to be used as a key in a weak store is that it have an identity, which in this case it would; its state isn't a consideration when it's being used as a key.

The reason there appears to be a problem in your example is because you are using makeWeakStore inconsistently. The value you are storing with init is an amount, but when you pull the value back out again with get you are treating it as if it was a foo.

[warner]

Oops. Ok, here's a different example.. still a bit contrived, but I think we could imagine our developers writing something like this and expecting it to work.

const bidNotifiers = makeWeakStore();

function representAucion(state) {
  const auction = {
    initialize() {
      state.currentBest = null;
      state.bids = [];
    },
    addBid(bid) {
      // state.bids.push(bid); // would this even work? doesn't trigger setter
      const bids = state.bids;
      bids.push(bid);
      state.bids = bids; // ugh
      auction.sort();
    },
    removeBid(bid) {
      const bids = state.bids;
      for (let i=0; i++; i < bids.length) {
        if (sameKey(bids[i], bid)) {
          bids.splice(i, 1);
        }
        state.bids = bids;
      }
    },
    sort() {
      const bids = state.bids;
      const amountsIndex = bids.map((bid, index) => [bid.getAmount(), index]);
      amountsIndex.sort((a,b) => b[0] - a[0]);
      state.bids = amountsIndex.map((bid, index) => bid);
    },
    results() {
      const bids = state.bids;
      const losers = bids.slice(0, bids.length-1);
      let winner;
      if (bids.length) {
        winner = bids[bids.length-1];
      }
      return { winner, losers };
    },
    close() {
      const { winner, losers } = auction.results();
      if (winner) {
        const notifier = bidNotifiers.get(winner);
        E(notifier).youWon();
      }
      losers.map(loser => E(bidNotifiers.get(loser)).youLost());
    },
  };
  return auction;
}

function representBid(state) {
  const bid = {
    initialize(auction, amount, notifier) {
      bidNotifiers.set(bid, notifier);
      state.amount = amount;
      auction.addBid(bid);
    },
  };
  return bid;
}

const auctionMaker = makeKind(representAucion);
const bidMaker = makeKind(representBid);

const auction = auctionMaker();
const newBid = bidMaker(auction, 20, notifier);

(the funny dance in addBid is a separate problem, if the mutation we want to make to state doesn't trigger the setter produced by wrapData, then the change will go unnoticed.. we should come back to this one later)

In this example, I think calling bidMaker would fail. bid.initialize does the obviously-correct thing and prepares itself completely before revealing itself to auction.addBid. Then auction.addBid does a sensible thing and adds the bid to it's state.bids before calling a common sort() function (imagine there is a changeBid method that would require a re-sort, so it would make sense to factor sort() out to a separate function).

sort() re-reads state.bid, which causes the wrapData getter to re-unserialize the data. That will create a new representative for our bid, which is troublesome because original amount hasn't been stored anywhere yet: it only exists on the innerSelf.rawData object being handled in the original makeRepresentative call. Then sort() calls bid.getAmount() to retrieve that data, and I think it won't be able to find it.

Looking at the current #1907 PR (848eda7), I think the stack will look like:

• makeNewInstance() (for newBid)
  • this creates innerSelf, let's call it innerSelf0
• initialRepresentative.initialize(...args) (for newBid)
• bid.initialize
• auction.addBid(newBid)
  • this adds newBid to state.bids, which gets serialized as the ID via valToSlot
• auction.sort()
  • when auction.sort() reads state.bids, the ID gets deserialized with this stack:
    • reanimate(ID) (for bid)
    • cache.lookup(instanceKey) is used to obtain an innerSelf, let's call it innerSelf1:
      • if the state.bids.length < cacheLength, then cache.lookup will hit the cache, and return the same innerSelf0 whose .rawData contains .amount = m.serialize(20). innerSelf0 === innerSelf1.
      • else if deserializing state.bids knocked innerSelf0 off the LRU, then cache.lookup will resort to creating a new innerObj, whose .rawData is obtained by calling fetch(instanceKey). innerSelf1 !== innerSelf0. The cache no longer remembers innerSelf0.
        • makeNewInstance hasn't gotten far enough to do anything with the 20, so innerSelf1.rawData.amount === undefined, so bid.getAmount() will fail
        • innerSelf0 is no longer tracked by the LRU, so when makeNewInstance finally gets control again and serializes initialData and assigns it to innerSelf.rawData, that data will only exist on initialRepresentative and won't be seen by any other users

This is super-thorny. I don't have a good suggestion on fixing it yet, other than to think about how we could simplify things (which is probably in conflict with providing an ergonomic API).

My one observation is that relying upon innerSelf to be tracked for a writeback means we must maintain the invariant that nothing can access the LRU cache (and cause innerSelf to get knocked out) between the time we do ensureState and the time we modify innertSelf.rawData. We can maintain that invariant in the getter/setter calls, but the hard part is maintaining it during makeNewInstance, given that the user's initialize method is allowed to do anything it wants.

[warner]

Ooh, @FUDCo and I just thought of a fix: add away to pin innerSelf0 to the LRU cache during initialization, so it can't be flushed until after the setup code has finished. Either an extra couple of methods and maybe a Map or WeakMap, or a special magic (Symbol) property on the innerSelf object that the LRU knock-out-of-cache method checks for before writeback.

[warner]

This might have been fixed as a side-effect of our big #4905 virtual-kind API change.

[FUDCo]

This might have been fixed as a side-effect of our big #4905 virtual-kind API change.

Yes. Closing.