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

Publish amm pool list subscription to chain storage #5400

Merged
merged 7 commits into from
Jun 4, 2022

Conversation

michaelfig
Copy link
Member

@michaelfig michaelfig commented May 19, 2022

refs: #4398
refs: #5386

Description

Created new makeStoredSubscription to produce a StoredSubscription<T> (a subscription with Merkle tree query information). Passed a storageNode and marshaller from bootstrap to the AMM contract for a new StoredSubscription<{ XYK: Brand[] }>.

Drive by:

  • reenabled CI build cache
  • allow synchronous throws in iterationObservers to terminate the iteration
  • fix makeScaledPriceAuthority to query its underlying authority just-in-time

Security Considerations

Documentation Considerations

Testing Considerations

Here's a demo:

snow:cosmic-swingset michael$ agd query swingset keys ''
keys:
- activityhash
- published
pagination: null
snow:cosmic-swingset michael$ agd query swingset keys published
keys:
- amm
pagination: null
snow:cosmic-swingset michael$ agd query swingset keys published.amm
keys:
- metrics
pagination: null
snow:cosmic-swingset michael$ agd query swingset keys published.amm.metrics
keys: []
pagination: null
snow:cosmic-swingset michael$ agd query swingset storage published.amm.metrics
value: '{"body":"{\"XYK\":[{\"@qclass\":\"slot\",\"iface\":\"Alleged: IbcATOM brand\",\"index\":0}]}","slots":["board0371"]}'
snow:cosmic-swingset michael$

@michaelfig michaelfig self-assigned this May 19, 2022
@michaelfig michaelfig changed the base branch from master to mfig-subscription-history-limit May 20, 2022 01:12
@michaelfig michaelfig force-pushed the mfig-subscription-history-limit branch from e0599e0 to bbbd125 Compare May 20, 2022 01:19
@michaelfig michaelfig changed the title Support for marshal publishers and subscribers Publish contract data to chain storage Jun 4, 2022
@michaelfig michaelfig changed the base branch from mfig-subscription-history-limit to gibson-4558-chainStorage June 4, 2022 01:50
@michaelfig michaelfig force-pushed the mfig-agoric-publish branch 2 times, most recently from 5d2f833 to e9c23d6 Compare June 4, 2022 05:39
@michaelfig michaelfig force-pushed the gibson-4558-chainStorage branch 2 times, most recently from f182c61 to f22b8f8 Compare June 4, 2022 05:59
Base automatically changed from gibson-4558-chainStorage to master June 4, 2022 10:41
@michaelfig michaelfig marked this pull request as ready for review June 4, 2022 15:48
@michaelfig michaelfig requested a review from turadg as a code owner June 4, 2022 15:48
@michaelfig michaelfig changed the title Publish contract data to chain storage Publish amm pool list subscription to chain storage Jun 4, 2022
@michaelfig michaelfig changed the title Publish amm pool list subscription to chain storage Publish amm pool list chain stream Jun 4, 2022
@michaelfig michaelfig changed the title Publish amm pool list chain stream Publish amm pool list subscription to chain storage Jun 4, 2022
@dckc dckc added the AMM label Jun 4, 2022
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.

fun stuff!

just a few tweaks to consider

packages/notifier/src/storesub.js Outdated Show resolved Hide resolved
packages/notifier/src/asyncIterableAdaptor.js Show resolved Hide resolved
packages/vats/src/lib-board.js Show resolved Hide resolved
} = makeSubscriptionKit();
const { storageNode, marshaller } = privateArgs;
const metricsStorageNode =
storageNode && E(storageNode).getChildNode('metrics'); // TODO: magic string
Copy link
Member

Choose a reason for hiding this comment

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

maybe change the "TODO: magic string" comment to a justification: 'metrics' is derived from getMetrics which is already part of the public API. I suppose it's still feasible to make a manifest constant.

Copy link
Member Author

Choose a reason for hiding this comment

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

I left this TODO intact, for a later date.

packages/run-protocol/src/vpool-xyk-amm/types.js Outdated Show resolved Hide resolved
mhofman
mhofman previously requested changes Jun 4, 2022
@@ -5,11 +5,6 @@ inputs:
node-version:
description: 'The version of Node.js to use'
required: true
cache-built:
Copy link
Member

Choose a reason for hiding this comment

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

This was needed for the deployment test job which couldn't use the build cache because it runs on a different linux environment, so the native modules would not match. The default value of true should have made it no impact on any other job. I need to understand why you claim it affected other jobs.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll create a new PR to distinguish the cache key without an explicit toggle.

@michaelfig michaelfig force-pushed the mfig-agoric-publish branch 2 times, most recently from fac70e7 to f9894f6 Compare June 4, 2022 18:14
@michaelfig michaelfig added the automerge:no-update (expert!) Automatically merge without updates label Jun 4, 2022
@michaelfig michaelfig dismissed mhofman’s stale review June 4, 2022 18:29

Objectionable change was removed from this PR.

@mergify mergify bot merged commit 308309e into master Jun 4, 2022
@mergify mergify bot deleted the mfig-agoric-publish branch June 4, 2022 19:12
t.fail(`unexpected finish with state ${state}`);
},
fail: reason => {
t.is(reason.message, 'sync propagates');
Copy link
Member

Choose a reason for hiding this comment

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

@michaelfig question: how does the code calling observeIteration differentiate between a failure in the iterator and an error thrown from its own listener?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not currently possible. The right behaviour would be: if you install a fail handler, it will only be called if the iterator publishes a failure. Listener throws should only be reflected by rejecting the outermost promise, and not call the fail handler.

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

Successfully merging this pull request may close these issues.

5 participants