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

Move AMM into a package that collects RUN protocol components #4218

Merged
merged 2 commits into from
Dec 30, 2021
Merged

Move AMM into a package that collects RUN protocol components #4218

merged 2 commits into from
Dec 30, 2021

Conversation

Chris-Hibbert
Copy link
Contributor

@Chris-Hibbert Chris-Hibbert commented Dec 27, 2021 •
edited
Loading

refs: #4199

Description

Move the AMM out of the Zoe contacts directory and into packages/run-protocol.

Security Considerations

This should be without security implications. It's just rearranging packages.

Documentation Considerations

As mentioned in #4199 and #4209, the docs need to be reorganized to make RUN protocol more visible.

Testing Considerations

Rearranging the test subdirectory to distinguish the various RUN protocol components.

Updates to Type Declarations

As a result of the rearrangement, eslint reported some type inconsistencies that weren't previously evident. I suspect that type checking is loose in .../zoe/src/contracts or .../zoe/tests, but tighter in run-protocol. In any case, I cleaned up some type declarations in .../src/. I also added some // @ts-ignore comments in tests that were constructing parameter lists on the fly by destructuring objects.

@Chris-Hibbert Chris-Hibbert added hygiene Tidying up around the house Core Economy OBSOLETE in favor of INTER-protocol Inter-protocol Overarching Inter Protocol AMM labels Dec 27, 2021
@Chris-Hibbert Chris-Hibbert added this to the Mainnet: Phase 1 - Treasury Launch milestone Dec 27, 2021
@Chris-Hibbert Chris-Hibbert self-assigned this Dec 27, 2021
@Chris-Hibbert Chris-Hibbert changed the base branch from master to renameTreasury December 27, 2021 22:13
@Chris-Hibbert Chris-Hibbert force-pushed the moveAmm branch 5 times, most recently from 687c4ed to 577a4d3 Compare December 28, 2021 01:22
@Chris-Hibbert Chris-Hibbert requested a review from dckc December 28, 2021 20:40
dckc
dckc requested changes Dec 28, 2021
View reviewed changes
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.

renaming test-stablecoin.js seems worth doing

packages/run-protocol/src/vpool-xyk-amm/types.js
Comment on lines +65 to +66
* @property {() => Brand[]} getAllPoolBrands
* @property {() => Allocation} getProtocolPoolBalance
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious what prompted this. It looks like a bit of a (welcome) drive-by.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't investigate, but I suspected that moving tests around caused a change to the way declarations were imported or how eslint was configured in tests. As I recall, we have eslint warnings turned off in some test directories.

packages/run-protocol/test/amm/constantProduct/test-checkInvariants.js
Comment on lines +16 to +22
* @typedef {Object} SwapPriceArgs
* @property {Amount} amountGiven
* @property {PoolAllocation} poolAllocation
* @property {Amount=} amountWanted
* @property {Ratio} protocolFeeRatio
* @property {Ratio} poolFeeRatio
*/
Copy link
Member

Choose a reason for hiding this comment

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

more (welcome) type tweaks... seems worth a mention in the commit message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a few lines to the message.

packages/run-protocol/test/amm/constantProduct/test-checkInvariants.js Outdated
@@ -110,6 +119,7 @@ function checkGetOutput(t, args, result) {

const testGetInputPrice = (t, inputs, runIn) => {
const args = runIn ? prepareRUNInTest(inputs) : prepareRUNOutTest(inputs);
// @ts-ignore
Copy link
Member

Choose a reason for hiding this comment

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

add a few words about why @ts-ignore is appropriate here?

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/constantProduct/test-compareBondingCurves.js Outdated
@@ -39,6 +39,7 @@ const prepareSwapInTest = ({ inputReserve, outputReserve, inputValue }) => {

const testInputGetPrice = (t, inputs, expectedOutput) => {
const { args, bld } = prepareSwapInTest(inputs);
// @ts-ignore
Copy link
Member

Choose a reason for hiding this comment

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

likewise for the other @ts-ignore occurrences

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/vaultFactory/test-stablecoin.js
@@ -18,11 +18,11 @@ import {
import { makePromiseKit } from '@agoric/promise-kit';
import { makeScriptedPriceAuthority } from '@agoric/zoe/tools/scriptedPriceAuthority.js';
import { assertAmountsEqual } from '@agoric/zoe/test/zoeTestHelpers.js';
import { makeParamManagerBuilder } from '@agoric/governance';
Copy link
Member

Choose a reason for hiding this comment

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

the name of this file should change, yes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point. I looked around and found a few files with treasury in their names and renamed them, too.

dckc
dckc approved these changes Dec 29, 2021
View reviewed changes
packages/run-protocol/test/amm/constantProduct/test-checkInvariants.js
@@ -119,7 +119,7 @@ function checkGetOutput(t, args, result) {

const testGetInputPrice = (t, inputs, runIn) => {
const args = runIn ? prepareRUNInTest(inputs) : prepareRUNOutTest(inputs);
// @ts-ignore
// @ts-ignore typescript doesn't like param list built by destructuring
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if there's a way to use Parameters<Type>.

I'm not inspired to work it out just now, though.

@Chris-Hibbert Chris-Hibbert added the automerge:squash Automatically squash merge label Dec 29, 2021
Base automatically changed from renameTreasury to master December 30, 2021 00:25
@Chris-Hibbert
refactor: move the AMM code from zoe to run-protocol
68b9c01
@Chris-Hibbert
chore: responses to review
af6710b 
renamed some files from treasury/stablecoin to vaultFactory
annotated some ts-ignore statements in tests.

regenerated png files in docs/threat_models
@mergify mergify bot merged commit a9895c4 into master Dec 30, 2021
@mergify mergify bot deleted the moveAmm branch December 30, 2021 00:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dckc dckc dckc approved these changes

Assignees

@Chris-Hibbert Chris-Hibbert

Labels
AMM automerge:squash Automatically squash merge Core Economy OBSOLETE in favor of INTER-protocol hygiene Tidying up around the house Inter-protocol Overarching Inter Protocol
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Chris-Hibbert @dckc