refactor(liveslots): minor improvements to prepare for upcoming changes

[erights]

closes: #XXXX
refs: endojs/endo#1964 endojs/endo#1966

Description

Minor refactors to prepare for upcoming changes.

Some minor typing improvements. Mostly being explicit that cache.get(key) can return an answer or undefined, and then adding explicit assert(thing !== undefined); statements as suggested by the type checker. I believe in all cases where I added these, the undefined case would have caused an early enough error anyway, so the only behavioral change would be to make the error a bit clearer.

Reviewers, please look at all these potential behavioral changes to be sure my added assertions would not cause any buggy behavior. Also, are some of these cases that should always have handled the undefined case differently than just throwing an error?

As previously discussed with @FUDCo, cache.delete(key) now returns a boolean to say whether it actually deleted anything. The motivation for this was receiveRevoker which is disappearing endojs/endo#1964 , but this seems like a good change anyway. It is more consistent with other collection.delete(key) APIs.

The most significant are the simplifications of deleting makeContextProvider and makeContextProviderKit, and putting their context provider making code inline.

Security Considerations

none

Scaling Considerations

none

Documentation Considerations

none

Testing Considerations

none

Upgrade Considerations

none

[FUDCo]

This all seems fine.

Having cache.delete(key) return a boolean seems like a strict improvement. It's information that's easily ignored if you don't care about it, but nice to have if you do care. Since pre-existing code was necessarily in the "don't care" category, there shouldn't be any backwards compatibility issues, which is the main thing I worry about when you change a return type.

[warner]

This won't work as expected (assuming that Map-like behavior is expected). The cache doesn't know whether a given key is present in the backing store or not, it only knows whether it was marked for deletion by a previous call to delete(). So the answer will depend upon the history of the cache as well as the backing store:

present in backing store	cache operations	expected result	actual result
yes	delete()	true	false
no	delete()	false	false
yes	get() + delete()	true	true
no	get() + delete()	false	true

Liveslots doesn't have a need for a delete() return value, and it would cost an extra DB query to make this act like Map, so I'd suggest reverting this one piece.

The other change (get() can return undefined but the typedef didn't reflect that) is absolutely correct.

PR #9509 is for reverting this part.. my apologies for not reviewing this in a timely fashion.