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

Add PSM upgrade for upgrade-19 #10730

Merged
merged 9 commits into from
Dec 30, 2024
Merged

Add PSM upgrade for upgrade-19 #10730

merged 9 commits into from
Dec 30, 2024

Conversation

iomekam
Copy link
Contributor

@iomekam iomekam commented Dec 18, 2024

closes: #10403

Description

Adds the upgrade of the psm contracts to upgrade 19. To ensure that this upgrade will succeed, we've also added test coverage in A3P.

Upgrade Considerations

This PR adds the PSM contract to the list of vat upgrades. The PSM upgrade proposal will upgrade all PSM instances on the chain. We've added A3P tests to ensure that the upgrade is successful and that assets are still in the reserve after the upgrade.

Copy link

cloudflare-workers-and-pages bot commented Dec 18, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: fc30b3e
Status: ✅  Deploy successful!
Preview URL: https://99cb57ec.agoric-sdk.pages.dev
Branch Preview URL: https://iomekam-psm-upgrade.agoric-sdk.pages.dev

View logs

@iomekam iomekam added the force:integration Force integration tests to run on PR label Dec 20, 2024
@iomekam iomekam marked this pull request as ready for review December 20, 2024 14:10
@iomekam iomekam requested a review from a team as a code owner December 20, 2024 14:10
@Chris-Hibbert
Copy link
Contributor

The Upgrade Considerations section is a copy of the one from #10541. Please consider the differences and re-write.

@Chris-Hibbert
Copy link
Contributor

I see that you included the force-integration flag, but it didn't actually run the Integration tests / test-docker-build test. Would you poke it again and see if you can get it to run these tests? I'd like to review the run output.

@@ -0,0 +1,80 @@
/* eslint-env node */
/**
* @file The goal of this file is to make sure v36-reserve upgraded.
Copy link
Contributor

Choose a reason for hiding this comment

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

copy-paste error

* 1. Simulate trade of IST and USDC
* 2. Upgrade all PSMs
* 3. Verify metrics are the same after the upgrade
* 4. Verity trading is still possible after the upgrade
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

Suggested change
* 4. Verity trading is still possible after the upgrade
* 4. Verify trading is still possible after the upgrade

};
});

test.serial('similate trade of IST and USDC', 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.

typo

Suggested change
test.serial('similate trade of IST and USDC', async t => {
test.serial('simulate trade of IST and USDC', async t => {

Comment on lines 16 to 17
console.log('[CONTRACT_KITS]', contractKits);
console.log('[ISSUER]', usdcIssuer);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why print the square brackets?

Comment on lines 50 to 51
console.log(await E(seat).getPayouts());
console.log('Done.');
Copy link
Contributor

Choose a reason for hiding this comment

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

please add a comment here that the success will be checked in the test by reading from vstorage.

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 made a suggestion that is totally optional.

Comment on lines +59 to +63
t.is(metrics.anchorPoolBalance.value, 500000n);
t.is(metrics.feePoolBalance.value, 0n);
t.is(metrics.mintedPoolBalance.value, 500000n);
t.is(metrics.totalAnchorProvided.value, 0n);
t.is(metrics.totalMintedProvided.value, 500000n);
Copy link
Contributor

Choose a reason for hiding this comment

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

I find it hard to compare these values visually with the previous and following values. What I've done in the past is use a variable to hold the mutating value, so the comparison always looks the same, and the changes appear explicitly near the place that changes the value.

In vaultFactoryTest, totalCollateral holds the evolving value, and it can be inserted in many different assertions

  await m.assertChange({
    totalCollateral: { value: totalCollateral },
  });

When it changes, there's an explicit assignment.

With 5 values changing in sync here, I'd define a record

  const poolMetrics = {
    anchorBalance: 500_000n,
    feeBalance: 0n
    mintedBalance: 500_000n,
    anchorProvided: 0n,
    mintedProvided: 500_000n,
};

and a comparer compareLevels(metrics, poolMetrics);. Then we could see that there's only one place where we change the values of poolMetrics

  poolMetrics.anchorBalance = poolMetrics.anchorBalance + 500_000n;
  poolMetrics.mintedBalance = poolMetrics.mintedBalance + 500_000n;
  poolMetrics.mintedProvided = poolMetrics.mintedProvided + 500_000n;

and otherwise, the values must be the same.

This particular case is on the very low end of the range where that would be called for, since the values only change once, but they're tested three times and there are 5 separate values compared, so I would probably make the change, but I'll leave it up to your judgement as to whether it's actually useful. This approach is most useful when the tests and outcomes keep changing, and this particular case might be stable already.

@iomekam iomekam added the automerge:squash Automatically squash merge label Dec 30, 2024
Copy link
Contributor

mergify bot commented Dec 30, 2024

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

@mergify mergify bot merged commit 3f302ba into master Dec 30, 2024
86 of 91 checks passed
@mergify mergify bot deleted the iomekam-psm-upgrade branch December 30, 2024 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:squash Automatically squash merge force:integration Force integration tests to run on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Contract Upgrade: 8 PSM contracts
2 participants