Agoric / agoric-sdk

monorepo for the Agoric Javascript smart contract platform
Apache License 2.0
326 stars 206 forks source link

Use `Proxy` for the state of Virtual Objects #5170

Open mhofman opened 2 years ago

mhofman commented 2 years ago

What is the Problem Being Solved?

Description of the Design

const init = ({ state, facets }, foo, notifier) => {
  state.foo = foo;
  observeNotifier(notifier,  facets.observer);
};

While this makes it easier for users to cause different instances to have different state shapes, especially adding entries after construction, it's already possible today at init time by conditionally setting state properties. I understand that we want to think of the state of virtual objects as a DB table, but we could also think of it as a big Map keyed by ${vref}:${stateProp}. This also doesn't prevent us in the future of adding an explicit schema that would be enforced by the proxy.

I believe by constructing the state once ahead of time, we can greatly simplify the VO internal init logic, since we don't need to extract and build a "manual getter/setter proxy" object after the fact.

Security Considerations

Not a security consideration per-se, but it's not obvious if the proxy trap should allow the state object to be sealed. However it's pretty clear that allowing the state to be frozen is likely undesirable, as it'd make the state unable to be updated. The proxy can prevent the state from being frozen or sealed.

Preventing state freezing would in effect prevent anything referencing the state from being hardened. That IMO is actually a benefit as the state should not end up used outside the behavior in the first place.

Test Plan

This would be a breaking API change, and I'm not sure if there's a way to provide ergonomic backwards compatibility.

erights commented 2 years ago

Attn @erights

erights commented 11 months ago
const init = ({ state, facets }, foo, notifier) => {

But IIUC we call (and must call) init before the facets (or self) are created. Yes? Are you suggesting instead that we make and provide the representatives using the state as their state while it is still an empty proxy and therefore typically violates all the state invariants the methods rely on?

I do not understand how this can play well with stateShape.

(from a private conversation)

it would allow us to get rid of finish

We have several exo finish functions in our code currently. For example in auctionBook.js, assetReserveKit.js, vaultManager.js, provisionPool.js, and smartWallet.js. Do you think that all of these finish functions would be unnecessary with this proposal?

mhofman commented 11 months ago

But IIUC we call (and must call) init before the facets (or self) are created. Yes? Are you suggesting instead that we make and provide the representatives using the state as their state while it is still an empty proxy and therefore typically violates all the state invariants the methods rely on?

init can only ever be called before facet methods are called. But facet objects can internally be created before init is called.

init would be in a position to call facet methods before it has populated the state that the methods expect, but that is an internal implementation concern of the exo.

I do not understand how this can play well with stateShape.

This issue was written before stateShape existed. However I do not believe that it is fundamentally incompatible with it. We could continue creating a state object with accessors based on the shape. One approach may be that for mandatory state properties without default values, the getters would throw until the property is first set, and that all their setters would need to be called before the state is considered ready. There's an interesting question about revoking the exo facets if init returns and the state wasn't populated entirely.

We have several exo finish functions in our code currently. Do you think that all of these finish functions would be unnecessary with this proposal?

I have not audited them but based on the purpose of finish, I do not see a reason why they couldn't be collapsed into the proposed init.

erights commented 11 months ago

init would be in a position to call facet methods before it has populated the state that the methods expect, but that is an internal implementation concern of the exo.

I see. In that case, I follow the rest. Thanks.

erights commented 11 months ago

One approach may be that for mandatory state properties without default values, the getters would throw until the property is first set, and that all their setters would need to be called before the state is considered ready.

This analogue of temporal dead zone cures my queasiness with the previous early-creation point. Invariant-violating uninitialized state would not be observable. Early calls to methods that read such state during init would fail during init. This is good.

Note that for shaped exos, we can still do all of this without a proxy. Just put that TDZ behavior into the accessors.