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

test(fast-usdc): test restart contract during long tx #10945

Merged
merged 1 commit into from
Feb 5, 2025

Conversation

samsiegart
Copy link
Contributor

@samsiegart samsiegart commented Feb 5, 2025

closes #10898
refs #10948

Description

  • Adds a contract restart to the middle advance bootstrap test
  • Enhances the existing restart test to change the feeConfig and verify changes in vstorage

Testing Considerations

Figured it was easier to just reuse the existing test case.

Upgrade Considerations

Gives us confidence that contract still functions during upgrade

@samsiegart samsiegart requested a review from a team as a code owner February 5, 2025 03:51
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.

Nice and clean. This got me thinking about another coverage we need.

@@ -503,14 +509,6 @@ test.serial('LP withdraws', async t => {
);
});

test.serial('restart contract', async t => {
Copy link
Member

Choose a reason for hiding this comment

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

The change above is good. I think as a matter of course we should name this test in each suite so it's clear that it's tested.

So please leave this here (with incarnationNumber: 2 and a comment as to why)

A bit ouf of scope but I think we also here should verify that not only does the contract keep working (as above) but things that are expected to change on restart do. I.e. feeConfig in the private args should show up in vstorage. (see writes fee config to vstorage test).

Please add that to this PR or make a separate issue to do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made an issue #10948

I'll do that as part of this PR then re-request review

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PTAL

Copy link

cloudflare-workers-and-pages bot commented Feb 5, 2025

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 6a37552
Status: ✅  Deploy successful!
Preview URL: https://3934abd2.agoric-sdk.pages.dev
Branch Preview URL: https://test-fu-restart.agoric-sdk.pages.dev

View logs

@samsiegart samsiegart requested a review from turadg February 5, 2025 20:34
owner: 'the updated fee configuration for Fast USDC after contract upgrade',
showValue: defaultSerializer.parse,
};
await documentStorageSchema(t, storage, doc);
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'm using documentStorageSchema because the remotable brands don't seem to play well with deepEqual

Copy link
Member

Choose a reason for hiding this comment

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

Worthwhile to write the snapshot but please first verify that storage has the new feeConfig.

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 tried:

  t.deepEqual(
    storage
      .getValues(`published.fastUsdc.feeConfig`)
      .map(defaultSerializer.parse)
      .at(-1),
    {
      flat: newFlat,
      variableRate: newVariableRate,
      contractRate: newContractRate,
    },
  );

And get:

Difference (- actual, + expected):

    {
      contractRate: {
        denominator: {
          brand: Object @Alleged: USDC brand {
  +         getKref: Function getKref {},
  +         iface: Function iface {},
          },
          value: 11n,
        },

I'm sure there's ways around it but I don't know why we'd need to clutter the test with that when the snapshot works fine. We can verify the snapshot in code review the same as we'd verify the assertion in the .ts file, and the test will fail the same way if it changes.

Copy link
Member

Choose a reason for hiding this comment

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

You can filter out brand.

why we'd need to clutter the test with that when the snapshot works fine

The snapshot is for detecting changes. The assertion here should be that the values being put in come back out.

I'm sure there's ways around it

Yes. For one you could filter out brand. We have work planned to make that unnecessary,

So if for now you want to leave a comment, I'll approve.

// XXX verify manually that the new feeConfig above appears in the snapshot
// UNTIL https://github.com/Agoric/agoric-sdk/issues/10491

@samsiegart samsiegart added the automerge:rebase Automatically rebase updates, then merge label Feb 5, 2025
@turadg turadg added the bypass:integration Prevent integration tests from running on PR label Feb 5, 2025
@mergify mergify bot merged commit 7b1656c into master Feb 5, 2025
84 of 93 checks passed
@mergify mergify bot deleted the test-fu-restart branch February 5, 2025 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge bypass:integration Prevent integration tests from running on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test of upgrading FU contract during a long transaction
2 participants