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

chore: parameterize price feed replacement (check liquidation) #9827

Closed
wants to merge 21 commits into from

Conversation

dckc
Copy link
Member

@dckc dckc commented Aug 1, 2024

DRAFT until:

  • fill in the right per-brand proposals
  • adjust setup, outcome constants

stacked on

was:

Description

/**
 * @file  The goal of this test is  to see that the
 * upgrade scripts re-wire all the contracts so new auctions and
 * price feeds are connected to vaults correctly.
 *
 * 1. enter a bid
 * 2. force prices to drop so a vault liquidates
 * 3. verify that the bidder gets the liquidated assets.
 */

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

@dckc
Copy link
Member Author

dckc commented Aug 1, 2024

runs like this so far:

...
  ─

  1. setupVaults; placeBids

  Error: Test finished without running any assertions



  run replace-price-feeds proposals
  Error: Test finished without running any assertions



  2. trigger liquidation by changing price

  test/bootstrapTests/price-feed-replace.test.ts:97

   96:   await advanceTimeTo(NonNullish(liveSchedule.nextDescendingStepTime));
   97:   t.like(readLatest(metricsPath), {                                    
   98:     numActiveVaults: 0,                                                

  Difference (- actual, + expected):

    {
      liquidatingCollateral: {
  -     value: 0n,
  +     value: 45000000n,
      },
      liquidatingDebt: {
  -     value: 0n,
  +     value: 309540000n,
      },
      lockedQuote: null,
  -   numActiveVaults: 1,
  +   numActiveVaults: 0,
  -   numLiquidatingVaults: 0,
  +   numLiquidatingVaults: 1,
    }

  › Object.value [as like] (file:///home/connolly/projects/agoric-sdk/node_modules/@endo/ses-ava/src/ses-ava-test.js:164:35)
  › <anonymous> (test/bootstrapTests/price-feed-replace.test.ts:97:5)



  3. verify liquidation
  test/bootstrapTests/price-feed-replace.test.ts:113

   112:   await advanceTimeBy(3, 'minutes');                                       
   113:   t.like(readLatest(`published.auction.book${managerIndex}`), {            
   114:     collateralAvailable: { value: scale6(setup.auction.start.collateral) },

  Difference (- actual, + expected):

    {
      collateralAvailable: {
  -     value: 0n,
  +     value: 45000000n,
      },
      startCollateral: {
  -     value: 0n,
  +     value: 45000000n,
      },
  -   startProceedsGoal: null,
  +   startProceedsGoal: {
  +     value: 309540000n,
  +   },
    }

  › Object.value [as like] (file:///home/connolly/projects/agoric-sdk/node_modules/@endo/ses-ava/src/ses-ava-test.js:164:35)
  › <anonymous> (test/bootstrapTests/price-feed-replace.test.ts:113:5)

  ─

  4 tests failed
error Command failed with exit code 1.

Copy link

cloudflare-workers-and-pages bot commented Aug 1, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 3d6c4e7
Status: ✅  Deploy successful!
Preview URL: https://9d61d2cb.agoric-sdk.pages.dev
Branch Preview URL: https://9584-up-vault-dc.agoric-sdk.pages.dev

View logs

@dckc dckc self-assigned this Aug 9, 2024
@dckc dckc force-pushed the 9584-up-vault-dc branch from 941b822 to 18f788e Compare August 9, 2024 21:54
@Chris-Hibbert Chris-Hibbert force-pushed the 9584-upgradeVaults branch 2 times, most recently from e69fe85 to 659c016 Compare August 12, 2024 22:37
@dckc dckc changed the title test: verify liquidation after replacing price feeds chore: parameterize price feed replacement (check liquidation) Aug 29, 2024
@dckc dckc changed the base branch from 9584-upgradeVaults to 9928-priceFeeds September 3, 2024 20:34
@dckc
Copy link
Member Author

dckc commented Sep 3, 2024

@Chris-Hibbert help me understand why liquidation is not triggering?

p.s. because the updated price is coming from a feed that vaults/auctions have yet to be introduced to.

✘ [fail]: 3. verify liquidation
$ yarn test test/bootstrapTests/price-feed-replace.test.ts
...

  ✘ [fail]: 3. verify liquidation


  3. verify liquidation

  test/bootstrapTests/price-feed-replace.test.ts:125

   124:   // vaultFactory sent collateral for liquidation
   125:   t.like(readLatest(metricsPath), {              
   126:     numActiveVaults: 0,                          

  Difference (- actual, + expected):

    {
      liquidatingCollateral: {
  -     value: 0n,
  +     value: 15000000n,
      },
      liquidatingDebt: {
  -     value: 0n,
  +     value: 100500000n,
      },
      lockedQuote: null,
  -   numActiveVaults: 1,
  +   numActiveVaults: 0,
  -   numLiquidatingVaults: 0,
  +   numLiquidatingVaults: 1,
    }

  › Object.value [as like] (file:///home/connolly/projects/agoric-sdk/node_modules/@endo/ses-ava/src/ses-ava-test.js:164:35)
  › <anonymous> (test/bootstrapTests/price-feed-replace.test.ts:125:5)



  1 test failed
error Command failed with exit code 1.

I rebased it on #9999. There's more to do, but I'd like to get this part working.

@dckc dckc force-pushed the 9584-up-vault-dc branch 2 times, most recently from 076c604 to f83e8c2 Compare September 9, 2024 19:01
@dckc dckc changed the base branch from 9928-priceFeeds to master September 9, 2024 19:03
Copy link

github-actions bot commented Sep 9, 2024

Base branch is changed to master. Please re-run the integration tests by adding 'force:integration' label.

@Chris-Hibbert Chris-Hibbert added the force:integration Force integration tests to run on PR label Sep 9, 2024
@dckc
Copy link
Member Author

dckc commented Sep 10, 2024

@Chris-Hibbert over to you as of 3d6c4e7

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.

I have two questions.

I saw that you addressed the missing consume power.

await priceFeedDrivers[collateralBrandKey].refreshInvitations();
});

test.serial('1. place bid', async t => {
Copy link
Contributor

Choose a reason for hiding this comment

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

@turadg has pointed out that test.serial() doesn't mean "must run in this order". It apparently means "must run one at a time in some order".

Should we do something else for tests that must run in a particular order?

Copy link
Member

Choose a reason for hiding this comment

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

when tests must run in a particular order, they're better thought of as one test. if you want more structure around the steps, a little helper can be made to log the progress.

Comment on lines +81 to +82
oracleBrandProduce[name].reset();
oracleBrandProduce[name].resolve(brand);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it okay for this to overwrite existing brands? Do we expect to use that ability?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably not.

provideInertBrand(name) gives the same result for the same name, so it shouldn't make any difference.

But the .reset() is probably a distraction.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll drop the reset(). Thanks.

Comment on lines 284 to 309
[deployPriceFeeds.name]: {
consume: {
agoricNamesAdmin: t,
board: t,
chainStorage: t,
chainTimerService: t,
econCharterKit: t,
highPrioritySendersManager: t,
namesByAddressAdmin: t,
priceAuthority: t,
priceAuthorityAdmin: t,
startGovernedUpgradable: t,
zoe: t,
},
instance: {
produce: t,
},
},
Copy link
Contributor

Choose a reason for hiding this comment

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

installPriceAggregator needs

    installation: {
      produce: { priceAggregator },
    },

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that a later commit handled this.

@Chris-Hibbert
Copy link
Contributor

This has been incorporated into #10074.

mergify bot added a commit that referenced this pull request Oct 9, 2024
closes: #9584
closes: #9928
refs: #9827 
refs: #9748 
refs: #9382
closes: #10031

## Description

We added upgrading the scaledPriceAuthority to the steps in upgrading vaults, auctions, and priceFeeds, and didn't notice that it broke things. The problem turned out to be that the "priceAuthority" object registered with the priceFeedRegistry was an ephemeral object that was not upgraded. This fixes that by re-registering the new priceAuthority.

Then, to simplify the process of cleaning up the uncollected cycles reported in #9483, we switched to replacing the scaledPriceAuthorities rather than upgrading them.

We also realized that we would need different coreEvals in different environments, since the Oracle addresses and particular assets vary for each (test and mainNet) chain environment.

#9748 addressed some of the issues in the original coreEval. #9999 showed what was needed for upgrading priceFeeds, which was completed by #9827.  #10021 added some details on replacing scaledPriceAuthorities.

### Security Considerations

N/A

### Scaling Considerations

Addresses one of our biggest scaling issues.

### Documentation Considerations

N/A

### Testing Considerations

Thorough testing in a3p, and our testnets.  #9886 discusses some testing to ensure Oracles will work with the upgrade.

### Upgrade Considerations

See above
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
force:integration Force integration tests to run on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants