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.

Sign up for GitHub

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

Jump to bottom

feat: publish econ stats from AMM #5420

Merged
merged 3 commits into from
May 24, 2022
Merged

feat: publish econ stats from AMM #5420

merged 3 commits into from
May 24, 2022

Conversation

Chris-Hibbert
Copy link
Contributor

closes: #4648

Description

Publish econ stats from the AMM

Security Considerations

Not a security issue

Documentation Considerations

Nothing new

Testing Considerations

Tested that updates are emitted for new pools and for changes due to trading and liquidity changes.

@Chris-Hibbert Chris-Hibbert added enhancement New feature or request Inter-protocol Overarching Inter Protocol AMM labels May 23, 2022
@Chris-Hibbert Chris-Hibbert added this to the Mainnet 1 milestone May 23, 2022
@Chris-Hibbert Chris-Hibbert requested a review from turadg as a code owner May 23, 2022 18:20
@Chris-Hibbert Chris-Hibbert self-assigned this May 23, 2022
turadg
turadg requested changes May 23, 2022
View reviewed changes
packages/run-protocol/src/vpool-xyk-amm/multipoolMarketMaker.js
const getPool = brand => secondaryBrandToPool.get(brand).pool;
const getPoolHelper = brand => secondaryBrandToPool.get(brand).helper;
const initPool = secondaryBrandToPool.init;
const isSecondary = secondaryBrandToPool.has;

const quoteIssuerKit = makeIssuerKit('Quote', AssetKind.SET);

const { publication: metricsPublication, subscription: metricsSubscription } =
Copy link
Member

Choose a reason for hiding this comment

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

please type this so there is API documentation on what comes out of getMetrics.

e.g. define a type up top,

/**
 * @typedef {object} MetricsNotification
 * @property {Brand[]} XYK brands
 */

then here would be,

Suggested change
const { publication: metricsPublication, subscription: metricsSubscription } =
/** @type {SubscriptonKit<MetricsNotification> */
const { publication: metricsPublication, subscription: metricsSubscription } =

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

packages/run-protocol/src/vpool-xyk-amm/multipoolMarketMaker.js
@@ -177,6 +188,9 @@ const start = async (zcf, privateArgs) => {
};
};

/** @param {Brand} brand */
const getPoolMetrics = brand => getPool(brand).getMetrics();
Copy link
Member

Choose a reason for hiding this comment

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

consider inlining

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

packages/run-protocol/src/vpool-xyk-amm/pool.js Outdated
Comment on lines 122 to 134
Central: facets.pool.getCentralAmount(),
Secondary: facets.pool.getSecondaryAmount(),
Liquidity: state.liqTokenSupply,
Copy link
Member

Choose a reason for hiding this comment

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

extant metrics are camelCase. Open to changing that but I think we should try to be consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These match the keywords, which are (uniquely) initial cap. I think it's worth supporting that exception, without relaxing camelCase as a general rule.

packages/run-protocol/src/vpool-xyk-amm/pool.js
@@ -295,6 +308,10 @@ export const definePoolKind = (
const { brand: liquidityBrand, issuer: liquidityIssuer } =
liquidityZcfMint.getIssuerRecord();
const { notifier, updater } = makeNotifierKit();
const {
Copy link
Member

Choose a reason for hiding this comment

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

please type

Suggested change
const {
/** @type {SubscriptionKit<MetricsNotification> */
const {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI, that's SubscriptionRecord<>. Took me a bit to find it.

packages/run-protocol/src/vpool-xyk-amm/pool.js Outdated
return 'Added liquidity.';
},
updateMetrics: ({ state, facets }) => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
updateMetrics: ({ state, facets }) => {
/** @param {MethodContext} context */
updateMetrics: ({ state, facets }) => {

this would reveal that metricsPublication and metricsSubscription need to be added to the ImmutableState type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

packages/run-protocol/src/vpool-xyk-amm/types.js Outdated
@@ -68,6 +68,7 @@
* @property {() => PriceAuthority} getToCentralPriceAuthority
* @property {() => PriceAuthority} getFromCentralPriceAuthority
* @property {() => VirtualPool} getVPool
* @property {() => Subscription<unknown>} getMetrics
Copy link
Member

Choose a reason for hiding this comment

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

all the unknown in this file are well known. please include the type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now they are.

packages/run-protocol/test/amm/vpool-xyk-amm/test-xyk-amm-swap.js Outdated
Comment on lines 450 to 453
const metricsNotifier = makeNotifierFromAsyncIterable(metricsSub);

const state0 = await E(metricsNotifier).getUpdateSince();
t.deepEqual(state0.value, { XYK: [] });
Copy link
Member

Choose a reason for hiding this comment

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

the test will be easier to follow using this helper,

Suggested change
const metricsNotifier = makeNotifierFromAsyncIterable(metricsSub);
const state0 = await E(metricsNotifier).getUpdateSince();
t.deepEqual(state0.value, { XYK: [] });
const m = await subscriptionTracker(t, metricsSub);
m.assertInitial({ XYK: [] });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

packages/run-protocol/test/amm/vpool-xyk-amm/test-xyk-amm-swap.js Outdated
Comment on lines 464 to 465
const state1 = await E(metricsNotifier).getUpdateSince(state0.updateCount);
t.deepEqual(state1.value, { XYK: [moolaR.brand] });
Copy link
Member

Choose a reason for hiding this comment

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

and this would be,

Suggested change
const state1 = await E(metricsNotifier).getUpdateSince(state0.updateCount);
t.deepEqual(state1.value, { XYK: [moolaR.brand] });
m.assertChange({ XYK: {"1": moolaR.brand } });

"1" to indicate that only the 1-index of the array changed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@Chris-Hibbert Chris-Hibbert force-pushed the 4648-AMMData branch 2 times, most recently from 2daaa7b to ed73c57 Compare May 23, 2022 22:50
turadg
turadg reviewed May 23, 2022
View reviewed changes
packages/run-protocol/src/vpool-xyk-amm/pool.js Outdated
Comment on lines 60 to 64
* @typedef {{
* Central: Amount,
* Secondary: Amount,
* Liquidity: bigint,
* }} PoolMetricsNotification
Copy link
Member

Choose a reason for hiding this comment

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

per our call,

Suggested change
* @typedef {{
* Central: Amount,
* Secondary: Amount,
* Liquidity: bigint,
* }} PoolMetricsNotification
* @typedef {object} PoolMetricsNotification
* @property {Amount} centralAmount
* @property {Amount} secondaryAmount
* @property {NatValue} liquidityTokens - outstanding tokens

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

turadg
turadg approved these changes May 23, 2022
View reviewed changes
Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

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

Approving contingent on the planned changes

@Chris-Hibbert Chris-Hibbert added the automerge:squash Automatically squash merge label May 24, 2022
@mergify mergify bot merged commit 87a9e62 into master May 24, 2022
@mergify mergify bot deleted the 4648-AMMData branch May 24, 2022 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dckc dckc dckc left review comments

@turadg turadg turadg approved these changes

Assignees

@Chris-Hibbert Chris-Hibbert

Labels
AMM automerge:squash Automatically squash merge enhancement New feature or request Inter-protocol Overarching Inter Protocol
Projects
None yet
Milestone
Mainnet 1 RC0
Development

Successfully merging this pull request may close these issues.

Expose key AMM economic data via subscription
3 participants
@Chris-Hibbert @turadg @dckc