Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

6765 publish vaultManager's price feed #6862

Merged

Conversation

turadg
Copy link
Member

@turadg turadg commented Jan 26, 2023

closes: #6765

Description

This makes the VaultManager publish changes to its price feed. The PriceAuthority returns a Notifier so this adds a makeStoredNotifier to pipe the prices to vstorage. I considered just having an observeNotifier but we have a commitment that a contract consumer can do its work without reading vstorage. So we need to continue the StoredFacet pattern that exposes a getPath(). I dropped getStoreKey though since that's deprecated.

In the course of making these changes, I need a collateralUnit during initState and getting it requires an await, which initState can't do. Since vaultManager is a singleton kind, it can be passed into prepareVaultManager and exist in closure. So this also has a refactor to move many initState params up to that prepare. cc @dtribble as it introduces arguments to addChildBaggage.

Best reviewed by commit.

Security Considerations

Similar to the scaling considerations, wrt undue load.

Scaling Considerations

This exposes a Notifier on VaultManager which means that anyone can ask for a price since any updateCount. Not so, explains @michaelfig below,

A Notifier only keeps the latest state, and all historical states are dropped. The updateCount cannot be used to index into past states, it is only used for the Notifier implementation to determine if the consumer needs to wait for the next value from the producer, or if the latest state is good enough.

Documentation Considerations

The list of storage nodes published by VaultFactory should be updated. Deferring to #6111

Testing Considerations

New test. No need to test the vstorage writes per se.

@turadg turadg requested review from dckc and michaelfig January 26, 2023 20:52
@turadg turadg linked an issue Jan 26, 2023 that may be closed by this pull request
@michaelfig
Copy link
Member

This exposes a Notifier on VaultManager which means that anyone can ask for a price since any updateCount. I think that means all updates have to be held eternally.

That's a common misconception. A Notifier only keeps the latest state, and all historical states are dropped. The updateCount cannot be used to index into past states, it is only used for the Notifier implementation to determine if the consumer needs to wait for the next value from the producer, or if the latest state is good enough.

Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test coverage for the quotes storage feature looks good.

I'm not confident I grok the 1st 2 commits (change to provideChildBaggage, init args) but it seems like the sort of thing that wouldn't survive our test suite if it were wrong.

I presume the scaling considerations in the PR are, as @michaelfig suggests, based on a misunderstanding.

@@ -469,12 +470,15 @@ const makeVaultDirector = defineDurableExoClassKit(
const { managerBaggages } = ephemera;
// alleged okay because used only as a diagnostic tag
const brandName = await E(collateralBrand).getAllegedName();
const collateralUnit = await unitAmount(collateralBrand);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't suppose there's a straightforward way to get the displayInfo earlier, is there?

@@ -78,3 +78,38 @@ test('storage keys', async t => {
'mockChainStorageRoot.vaultFactory.manager0.vaults.vault2',
);
});

test('quotes storage', async t => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice test. thanks.

@turadg turadg added automerge:no-update (expert!) Automatically merge without updates and removed automerge:no-update (expert!) Automatically merge without updates labels Jan 26, 2023
@turadg turadg force-pushed the 6765-publish-price-feeds-of-tradeable-brands-per-vault-manager branch from c133778 to bad4226 Compare January 26, 2023 23:13
@turadg turadg added the automerge:no-update (expert!) Automatically merge without updates label Jan 26, 2023
Comment on lines +162 to +170
export const makeMarshallToStorage = (storageNode, marshaller) => {
return value =>
E(marshaller)
.serialize(value)
.then(serialized => {
const encoded = JSON.stringify(serialized);
return E(storageNode).setValue(encoded);
});
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, these names...

Suggested change
export const makeMarshallToStorage = (storageNode, marshaller) => {
return value =>
E(marshaller)
.serialize(value)
.then(serialized => {
const encoded = JSON.stringify(serialized);
return E(storageNode).setValue(encoded);
});
};
export const makeSerializeToStorage = (storageNode, marshaller) => {
return async value => {
const marshalled = await E(marshaller).serialize(value);
const serialized = JSON.stringify(marshalled);
return E(storageNode).setValue(serialized);
};
};

But also, what is the benefit of defining such a function in lib-chainStorage? I'd be happiest if this package were imported only from the vat that calls makeChainStorageRoot (i.e., the one with access to the bridge device) to which other vats just sent simple messages.

Also, if sending an object to a remote Marshaller to get input for JSON.stringify serialization on the way to chain storage is a common pattern, then I'd also like to save round trips by incorporating the JSON.stringify serialization into a logical wrapper of either Marshaller serialize or [less preferably] StorageNode setValue and getting the initiator out of the path between them, e.g. storageNode.setValue(E(marshaller).marshalAndSerialize(value))12 or storageNode.serializeAndSetValue(E(marshaller).serialize(value))).

Footnotes

  1. Extending StorageNode setValue's input signature from (value: string) to (value: ERef<string>).

  2. Where marshalAndSerialize takes a passable, encodes it (to CapData/smallcaps/etc.), serializes the result (to JSON text), and returns the result—and could/would be more tersely named serialize if that were not currently claimed by just the first [encoding] step.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on the names. I can't invest in until after Vault release.

The code change LGTM and I'll probably incorporate it into another PR. Not worth retriggering deployment-test rn.

The round trips is a problem. I think @dckc 's meant to help #6646

@mergify mergify bot merged commit c05c56e into master Jan 27, 2023
@mergify mergify bot deleted the 6765-publish-price-feeds-of-tradeable-brands-per-vault-manager branch January 27, 2023 00:22
@dckc
Copy link
Member

dckc commented Jan 27, 2023

re "The Endo one-interface-to-rule-them-all" for notifiers / subscriptions, I hope there's an issue to represent it. endojs/endo#1035 looks close. I'm inclined to use that until I hear otherwise.

@turadg turadg mentioned this pull request Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:no-update (expert!) Automatically merge without updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

publish price feeds of tradeable brands per vault manager
4 participants