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

feat(amm)!: push reserved assets to reserve contract #5429

Merged
merged 26 commits into from
May 27, 2022

Conversation

turadg
Copy link
Member

@turadg turadg commented May 25, 2022

closes: #5408
closes: #5271

Description

When the AMM makes a pool, add the collateral to the reserve.

This requires that the reserve has the issuer, so when an issuer is added to AMM, also add it to the reserve. Put this on the public facet so the AMM doesn't need special access, but use a special method so the the reserve validates it back against the AMM.

Security Considerations

New method on AMM creator facet to set the reserve instance

Documentation Considerations

Requires AMM to have resolveReserveFacet called before pools are created.

Testing Considerations

New tests. Updated bootstrap. Made some test config more consistent.

@turadg turadg force-pushed the 5408-reserve-gets-amm-liquidity branch 2 times, most recently from 2b66bee to 1d82acd Compare May 25, 2022 21:47
@turadg turadg requested review from Chris-Hibbert and dckc May 25, 2022 21:49
@turadg turadg force-pushed the 5408-reserve-gets-amm-liquidity branch from 1d82acd to c822c99 Compare May 26, 2022 15:07
turadg added 7 commits May 26, 2022 13:09
after:
```
  gov-collateral › assets are in AMM, Vaults

    ℹ && task was past its deadline when scheduled: 0 &&
    ℹ @@ schedule task for:86400, currently: 0 @@
    ℹ && task was past its deadline when scheduled: 0 &&
    ℹ @@ schedule task for:3600, currently: 0 @@
    ℹ addAsset {
        denom: 'ibc/abc123',
        issuer: '[object Alleged: ibc/abc123 issuer]',
        keyword: 'ibc/abc123',
      }

  Rejected promise returned by test. Reason:

  Error {
    message: 'Must first setReserveDepositFacet',
  }
```

before:
```

  gov-collateral › assets are in AMM, Vaults

    ℹ chainTimerService: new Promise
    ℹ zoe: new Promise
    ℹ feeMintAccess: new Promise
    ℹ agoricNames: new Promise
    ℹ agoricNamesAdmin: new Promise
    ℹ chainTimerService settled; remaining: [
        'agoricNames',
        'agoricNamesAdmin',
        'feeMintAccess',
        'zoe',
      ]
    ℹ zoe settled; remaining: [
        'agoricNames',
        'agoricNamesAdmin',
        'feeMintAccess',
      ]
    ℹ feeMintAccess settled; remaining: [
        'agoricNames',
        'agoricNamesAdmin',
      ]
    ℹ agoricNames settled; remaining: [
        'agoricNamesAdmin',
      ]
    ℹ agoricNamesAdmin settled; remaining: []
    ℹ loadVat: new Promise
    ℹ restoreBundleID: new Promise
    ℹ board: new Promise
    ℹ loadVat settled; remaining: [
        'board',
        'restoreBundleID',
      ]
    ℹ restoreBundleID settled; remaining: [
        'board',
      ]
    ℹ bridgeManager: new Promise
    ℹ bankManager: new Promise
    ℹ initialSupply: new Promise
    ℹ bldIssuerKit: new Promise
    ℹ client: new Promise
    ℹ clientCreator: new Promise
    ℹ coreEvalBridgeHandler: new Promise
    ℹ bridgeManager settled; remaining: [
        'bankManager',
        'bldIssuerKit',
        'board',
        'client',
        'clientCreator',
        'coreEvalBridgeHandler',
        'initialSupply',
      ]
    ℹ bankManager settled; remaining: [
        'bldIssuerKit',
        'board',
        'client',
        'clientCreator',
        'coreEvalBridgeHandler',
        'initialSupply',
      ]
    ℹ clientCreator settled; remaining: [
        'bldIssuerKit',
        'board',
        'client',
        'coreEvalBridgeHandler',
        'initialSupply',
      ]
    ℹ client settled; remaining: [
        'bldIssuerKit',
        'board',
        'coreEvalBridgeHandler',
        'initialSupply',
      ]
    ℹ namesByAddress: new Promise
    ℹ namesByAddressAdmin: new Promise
    ℹ coreEvalBridgeHandler settled; remaining: [
        'bldIssuerKit',
        'board',
        'initialSupply',
        'namesByAddress',
        'namesByAddressAdmin',
      ]
    ℹ namesByAddress settled; remaining: [
        'bldIssuerKit',
        'board',
        'initialSupply',
        'namesByAddressAdmin',
      ]
    ℹ namesByAddressAdmin settled; remaining: [
        'bldIssuerKit',
        'board',
        'initialSupply',
      ]
    ℹ initialSupply settled; remaining: [
        'bldIssuerKit',
        'board',
      ]
    ℹ board settled; remaining: [
        'bldIssuerKit',
      ]
    ℹ bldIssuerKit settled; remaining: []
    ℹ priceAuthorityVat: new Promise
    ℹ priceAuthority: new Promise
    ℹ priceAuthorityAdmin: new Promise
    ℹ priceAuthority settled; remaining: [
        'priceAuthorityAdmin',
        'priceAuthorityVat',
      ]
    ℹ priceAuthorityAdmin settled; remaining: [
        'priceAuthorityVat',
      ]
    ℹ priceAuthorityVat settled; remaining: []
    ℹ vaultFactoryCreator: new Promise
    ℹ economicCommitteeCreatorFacet: new Promise
    ℹ ammCreatorFacet: new Promise
    ℹ ammGovernorCreatorFacet: new Promise
    ℹ reserveCreatorFacet: new Promise
    ℹ reserveGovernorCreatorFacet: new Promise
    ℹ reservePublicFacet: new Promise
    ℹ lienBridge: new Promise
    ℹ runStakeCreatorFacet: new Promise
    ℹ runStakeGovernorCreatorFacet: new Promise
    ℹ vaultFactoryGovernorCreator: new Promise
    ℹ vaultFactoryVoteCreator: new Promise
    ℹ economicCommitteeCreatorFacet settled; remaining: [
        'ammCreatorFacet',
        'ammGovernorCreatorFacet',
        'lienBridge',
        'reserveCreatorFacet',
        'reserveGovernorCreatorFacet',
        'reservePublicFacet',
        'runStakeCreatorFacet',
        'runStakeGovernorCreatorFacet',
        'vaultFactoryCreator',
        'vaultFactoryGovernorCreator',
        'vaultFactoryVoteCreator',
      ]
    ℹ && task was past its deadline when scheduled: 0 &&
    ℹ @@ schedule task for:86400, currently: 0 @@
    ℹ runStakeGovernorCreatorFacet settled; remaining: [
        'ammCreatorFacet',
        'ammGovernorCreatorFacet',
        'lienBridge',
        'reserveCreatorFacet',
        'reserveGovernorCreatorFacet',
        'reservePublicFacet',
        'runStakeCreatorFacet',
        'vaultFactoryCreator',
        'vaultFactoryGovernorCreator',
        'vaultFactoryVoteCreator',
      ]
    ℹ runStakeCreatorFacet settled; remaining: [
        'ammCreatorFacet',
        'ammGovernorCreatorFacet',
        'lienBridge',
        'reserveCreatorFacet',
        'reserveGovernorCreatorFacet',
        'reservePublicFacet',
        'vaultFactoryCreator',
        'vaultFactoryGovernorCreator',
        'vaultFactoryVoteCreator',
      ]
    ℹ ammGovernorCreatorFacet settled; remaining: [
        'ammCreatorFacet',
        'lienBridge',
        'reserveCreatorFacet',
        'reserveGovernorCreatorFacet',
        'reservePublicFacet',
        'vaultFactoryCreator',
        'vaultFactoryGovernorCreator',
        'vaultFactoryVoteCreator',
      ]
    ℹ ammCreatorFacet settled; remaining: [
        'lienBridge',
        'reserveCreatorFacet',
        'reserveGovernorCreatorFacet',
        'reservePublicFacet',
        'vaultFactoryCreator',
        'vaultFactoryGovernorCreator',
        'vaultFactoryVoteCreator',
      ]
    ℹ reserveGovernorCreatorFacet settled; remaining: [
        'lienBridge',
        'reserveCreatorFacet',
        'reservePublicFacet',
        'vaultFactoryCreator',
        'vaultFactoryGovernorCreator',
        'vaultFactoryVoteCreator',
      ]
    ℹ reserveCreatorFacet settled; remaining: [
        'lienBridge',
        'reservePublicFacet',
        'vaultFactoryCreator',
        'vaultFactoryGovernorCreator',
        'vaultFactoryVoteCreator',
      ]
    ℹ reservePublicFacet settled; remaining: [
        'lienBridge',
        'vaultFactoryCreator',
        'vaultFactoryGovernorCreator',
        'vaultFactoryVoteCreator',
      ]
    ℹ vaultFactoryCreator settled; remaining: [
        'lienBridge',
        'vaultFactoryGovernorCreator',
        'vaultFactoryVoteCreator',
      ]
    ℹ vaultFactoryGovernorCreator settled; remaining: [
        'lienBridge',
        'vaultFactoryVoteCreator',
      ]
    ℹ vaultFactoryVoteCreator settled; remaining: [
        'lienBridge',
      ]
    ℹ && task was past its deadline when scheduled: 0 &&
    ℹ @@ schedule task for:3600, currently: 0 @@
    ℹ addAsset {
        denom: 'ibc/abc123',
        issuer: '[object Alleged: ibc/abc123 issuer]',
        keyword: 'ibc/abc123',
      }

  Rejected promise returned by test. Reason:

  Error {
    message: 'Must first setReserveDepositFacet',
  }
```
@turadg turadg force-pushed the 5408-reserve-gets-amm-liquidity branch from c822c99 to fd44fca Compare May 26, 2022 20:19
);

const secondaryIssuer = await E(ammPublicFacet).getIssuer(secondaryBrand);
// ??? why don't these objects have methods?
Copy link
Member Author

Choose a reason for hiding this comment

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

@Chris-Hibbert @dckc do you know a way to get objects that match the typedef for Brand and/or Issuer? Then I could use getAllegedName() to make the tests more readable.

Copy link
Member

Choose a reason for hiding this comment

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

are the objects remote? you should be able to use E(brand).getAllegedName().

@@ -51,9 +51,22 @@ const start = async (zcf, privateArgs) => {
/**
* Used to look up the brands for keywords, excluding RUN because it's a special case.
*
* @type {MapStore<Keyword, [brand: Brand, liquidityBrand: Brand]>}
Copy link
Member Author

Choose a reason for hiding this comment

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

@Chris-Hibbert given the requirement that AddCollateral invitation accept any brand, I thought it cleanest to make these maps ignore the liquidity distinction. instead that's handled through a separate lookup between brands.

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.

my brain is kinda polluted with other stuff; I hope @Chris-Hibbert can get to this soon

@@ -261,6 +265,9 @@ export const setupReserve = async ({
},
},
}) => {
trace('setupReserve');
await reserveInstallation;
Copy link
Member

Choose a reason for hiding this comment

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

what's this for? why await it and then throw away the resolution?

Copy link
Member Author

Choose a reason for hiding this comment

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

earlier feedback on missing producers. I'll do some cleanup since they're confusing

Comment on lines 806 to 811
const reserveInstance = await space.instance.consume.reserve;
trace({ reserveInstance });
const reservePublicFacet = await E(space.consume.zoe).getPublicFacet(
reserveInstance,
);
const ammCreatorFacet = await space.consume.ammCreatorFacet;
Copy link
Member

Choose a reason for hiding this comment

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

These awaits aren't necessary. I can imagine they were helpful for debugging... and I guess this isn't performance-sensitive stuff... so I guess we might as well leave them there to facilitate future debugging.

Copy link
Member Author

Choose a reason for hiding this comment

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

exactly

);

const secondaryIssuer = await E(ammPublicFacet).getIssuer(secondaryBrand);
// ??? why don't these objects have methods?
Copy link
Member

Choose a reason for hiding this comment

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

are the objects remote? you should be able to use E(brand).getAllegedName().

Copy link
Contributor

@Chris-Hibbert Chris-Hibbert left a comment

Choose a reason for hiding this comment

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

Not a final review, just responses to questions, and initial reactions

@@ -630,6 +645,8 @@ test('price drop', async t => {
shortfallBalance: AmountMath.makeEmpty(runBrand),
});
await m.assertChange({
// FIXME expects to know liquidity brand
Copy link
Contributor

Choose a reason for hiding this comment

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

The test can look up the liquidityBrand from the AMM. The AMM is in scope
(services.ammFacets.ammPublicFacet).

@@ -47,10 +49,15 @@ test('startInstance', async t => {
},
};

/** @type {Petname=} */
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this drive-by fix fit in another PR?

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 don't know where it would go. I was burning down my problems pane.

Prefer that I take it out? would a separate commit in this PR with "merge" instead of squash suffice?

Copy link
Member

Choose a reason for hiding this comment

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

separate commit would be nice. don't take it out, is my preference

Copy link
Member Author

Choose a reason for hiding this comment

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

pulled non-run-protocol changes out to 66fffd9 so I can squash this PR.

/**
* @param {Brand} secondaryBrand
*/
const addIssuerFromAmm = async secondaryBrand => {
Copy link
Contributor

Choose a reason for hiding this comment

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

secondaryBrand is really the AMM's concept. I realize the Reserve knows that these brands are not the AMM's centralBrand, but after initialization, the Reserve doesn't treat them differently. I'd prefer that the param name here either be brand or ammSecondaryBrand. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

agreed it's an AMM concept. I thought being within method scoped names to AMM but I take your point. I do want to be clear that this is only for secondary brands from AMM and not liquidity brands, so i'll go with ammSecondaryBrand.

assert(
secondaryBrand !== runKit.brand,
`${RunKW} is a special case handled by the reserve contract`,
);
Copy link
Contributor

Choose a reason for hiding this comment

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

If addIssuerFromAMM is public, it has to verify that the brand is a secondary brand (not a liquidity brand) with the AMM before adding it. Otherwise, someone could bollix up our careful plans by calling addIssuerFromAmm(doubloonLiquidityBrand).
Why doesn't it work to add both issuers when someone (the AMM?) calls addIssuerFromAmm?

Copy link
Member Author

Choose a reason for hiding this comment

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

If addIssuerFromAMM is public, it has to verify that the brand is a secondary brand (not a liquidity brand) with the AMM before adding it. Otherwise, someone could bollix up our careful plans by calling addIssuerFromAmm(doubloonLiquidityBrand).

Had to read this a few times. Repeating what I got:

  • this design has addIssuerFromAMM public which means it has to verify the brand argument is a secondary brand with the AMM before adding it
  • the line const issuer = await E(ammPublicFacet).getIssuer(ammSecondaryBrand); can return a liquidity brand
  • therefore someone could bollix up our careful plans by calling addIssuerFromAmm(doubloonLiquidityBrand).

Why doesn't it work to add both issuers when someone (the AMM?) calls addIssuerFromAmm?

IIRC, the liquidity issuer wasn't yet defined on the AMM so the E(ammPublicFacet).getLiquidityIssuer call would fail.

Let's chat about whether we can simplify this.

Comment on lines 255 to 256
// DISCUSSION: this gumms up all the testing setup to require the reserve to be created AND set onto the AMM
// before the AMM can add an issuer
Copy link
Contributor

Choose a reason for hiding this comment

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

If the Reserve (or some intermediary) pulled the liquidity tokens later, then the AMM wouldn't have to be sure that the Reserve was ready for the Liquidity tokens when the pool was created.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. The "just-in-time" design had that. I thought you pressed for the caller having to set things up before sending liquidity to the reserve.

I think it's moot now as I've done the plumbing to make the AMM have the reserve early enough. I think it's better overall as it ensures a "valid" state of the contract earlier.

packages/run-protocol/test/amm/vpool-xyk-amm/setup.js Outdated Show resolved Hide resolved
Comment on lines +24 to +25
const farZoeKit = await setUpZoeForTest();
const { zoe } = farZoeKit;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this an orthogonal fix, or is it somehow associated with the reserve changes?

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 had to do a lot of bootstrap cleanup to get things loading in the right order. I don't remember whether this one was strictly necessary.

* @param {AssetReservePublicFacet} facet
*/
setReserveDepositFacet: facet => {
// FIXME more than deposits
Copy link
Member Author

Choose a reason for hiding this comment

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

for now how about simply setReservePublicFacet?

@@ -206,7 +206,8 @@ test('governance add Liquidity to the AMM', async t => {
'should be 80K',
);

await E(reserve.reserveCreatorFacet).addIssuer(moolaR.issuer, 'Moola');
// FIXME handle repeats
Copy link
Member Author

Choose a reason for hiding this comment

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

conjureKeyword seems to work well. I'll remove this comment in next push if you agree.

@turadg turadg requested review from dckc and Chris-Hibbert May 26, 2022 23:46
@turadg turadg marked this pull request as ready for review May 26, 2022 23:46
@turadg turadg requested a review from dtribble as a code owner May 26, 2022 23:46
@Chris-Hibbert
Copy link
Contributor

Chris-Hibbert commented May 26, 2022

I added closes: #5271 in the top comment.

@turadg
Copy link
Member Author

turadg commented May 27, 2022

I added closes: #5271 in the top comment.

If that auto-closes it may look like "Reserve's addIssuer … public" is the state of master. Since we decided to take another approach, would it be better to close that with a comment on why?

Copy link
Contributor

@Chris-Hibbert Chris-Hibbert left a comment

Choose a reason for hiding this comment

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

Getting close. A few minor requests for changes.

/**
* @type {MapStore<Brand, Brand>}
*/
const liquidityBrandForBrand = makeStore('liquidityBrands');
Copy link
Contributor

Choose a reason for hiding this comment

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

The argument to makeStore() is supposed to describe the key, not the value.

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 sure how to distinguish then between two maps with the same keys but different values.

leaving for now since the maps in master describe the value, not the keyword https://github.com/Agoric/agoric-sdk/blob/master/packages/run-protocol/src/reserve/assetReserve.js#L50-L56

Comment on lines +110 to +112
/**
* @param {Issuer} baseIssuer on which the liquidity issuer is based
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/**
* @param {Issuer} baseIssuer on which the liquidity issuer is based
*/
/** @param {Issuer} issuer on which the liquidity issuer is based */

Copy link
Member Author

Choose a reason for hiding this comment

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

baseIssuer isn't part of the sentence. Here's how it looks in an IDE:
Screen Shot 2022-05-27 at 6 41 52 AM

If you're proposing to rename the parameter, I think that would make the body less clear. I could add a dash to separate, but I think that's a style decision we should perform universally, not ad hoc in PR review. I could make it baseIssuer issuer on which if you think that would be better.

Copy link
Contributor

Choose a reason for hiding this comment

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

stet

Copy link
Member Author

Choose a reason for hiding this comment

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

TIL stet!

*/
setReserveDepositFacet: facet => {
// FIXME more than deposits
reserveDepositFacet = facet;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should assert !reserveDepositFacet before setting.

I'd prefer that this facet was visible via governance, but failing that, it should be set-once.

'and got back',
issuer,
);
// this ensures that getSecondaryIssuer will return
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// this ensures that getSecondaryIssuer will return
// this ensures that getSecondaryIssuer(thisIssuer) will return even before the pool is created

Comment on lines 62 to 63
// ??? still need this deferred lookup?
const addIssuerToReserve = getAddIssuerToReserve();
Copy link
Contributor

Choose a reason for hiding this comment

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

yes, I believe so. It also makes it simpler to transition to a governed parameter if we decide to go that way.

Comment on lines 254 to 256
/**
*
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

???

Comment on lines +53 to +56
farZoeKit,
) => {
if (!zoe) {
({ zoe } = await setUpZoeForTest());
if (!farZoeKit) {
farZoeKit = await setUpZoeForTest();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be simplified back out, too? [I thought you said similar code was added elsewhere to assist in debugging, but was no longer useful.]

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 don't think so. In this case the need for farZoeKit had to be wired through.

@@ -141,13 +142,17 @@ const setupAmmAndElectorate = async (t, aethLiquidity, runLiquidity) => {
const space = setupBootstrap(t, timer);
const { consume, instance } = space;
installGovernance(zoe, space.installation.produce);
// TODO
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO what?

Copy link
Member Author

Choose a reason for hiding this comment

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

hehe. I remember that was for DRYing out the next two lines. I plan to use

export const produceInstallations = (space, installations) => {
for (const [key, installation] of Object.entries(installations)) {
space.installation.produce[key].resolve(installation);
}
};

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 save that for another PR. WDYT of it @dckc ?

Copy link
Member

Choose a reason for hiding this comment

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

it's a reasonable idiom... I'm not sure it carries its weight as an exported function, where a reader has to switch context to learn what it does.

Presuming const { entries } = Object, it's a one-liner:

  entries(installations).forEach(([key, installation]) => space.installation.produce[key].resolve(installation));

I don't have much of a problem making the function available, but I think there's a significant risk that folks will open-code it rather than remember that it's available as a helper function.

The helper function should take just the space.installation part, not the whole space. POLA.

@turadg turadg changed the title feat(amm): push reserved assets to reserve contract feat(amm)!: push reserved assets to reserve contract May 27, 2022
@turadg turadg added the automerge:squash Automatically squash merge label May 27, 2022
@turadg turadg requested a review from Chris-Hibbert May 27, 2022 15:07
@Chris-Hibbert
Copy link
Contributor

I added closes: #5271 in the top comment.

If that auto-closes it may look like "Reserve's addIssuer … public" is the state of master. Since we decided to take another approach, would it be better to close that with a comment on why?

I don't have strong opinions. It's clear that merging this, makes that issue moot. Feel free to remove the closes: marker. I justs wanted to remember to close the other.

Copy link
Contributor

@Chris-Hibbert Chris-Hibbert left a comment

Choose a reason for hiding this comment

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

Minor comments. I trust your judgement on whether to address them or leave them as is.


const electorateTerms = { committeeName: 'EnBancPanel', committeeSize: 3 };
// This timer is only used to build quotes. Let's make it non-zero
const timer = buildManualTimer(console.log, 30n);
Copy link
Contributor

Choose a reason for hiding this comment

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

When the timer won't be used again in the current scope, I prefer to initialize it inline in the call (line 344) so as not to add its name to the local scope. In this case, there's a default value, so it's just as good to let it default, so it's never visible here. electorateTerms can also be moved into the call.

Comment on lines 294 to 297
// // make the installation available for setupReserve
// iProduce.reserve.resolve(t.context.installation.reserve);
// // produce the reserve instance in the space
// await setupReserve(space);
Copy link
Contributor

Choose a reason for hiding this comment

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

???

@@ -267,7 +267,6 @@ export const setupReserve = async ({
},
}) => {
trace('setupReserve');
console.trace('setupReserve');
Copy link
Member

Choose a reason for hiding this comment

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

ah. that makes sense.

Perhaps the console provided by SwingSet doesn't have .trace.

Copy link
Member Author

Choose a reason for hiding this comment

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

ahh

@mergify mergify bot merged commit 20472a1 into master May 27, 2022
@mergify mergify bot deleted the 5408-reserve-gets-amm-liquidity branch May 27, 2022 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:squash Automatically squash merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reserve collects liquidity held by AMM from creating new pools Reserve's addIssuer should be public
3 participants