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(fast-usdc): core-eval to change fastUsdc.feedPolicy #10609

Merged
merged 8 commits into from
Dec 8, 2024

Conversation

dckc
Copy link
Member

@dckc dckc commented Dec 3, 2024

closes: #10507

Description

  • core-eval to update fast-usdc feed policy
  • policy update support in init-fast-usdc builder script
  • some refactoring to make stuff available to the 2nd core-eval script and to the init-fast-usdc builder

Testing Considerations

1 happy-path test:

  • test(boot): core-eval to change fastUsdc.feedPolicy

Aside from option parsing in the builder, validation with patterns makes for essentially straight-line code.

Security / Documentation Considerations

The whole feedPolicy, for all chains, is updated at once. If the goal is to change a property for just 1 chain, it's important to keep the properties in tact for all the other chains.

Scaling Considerations

none

Upgrade Considerations

There has been some discussion of versions for feedPolicy. This PR doesn't add anything in that area. The block height of the vstorage write can be used as one form of version number, though that isn't available to the contract, so it wouldn't let the contract detect OCW reports based on outdated policies.

@dckc dckc requested a review from a team as a code owner December 3, 2024 06:05
@dckc
Copy link
Member Author

dckc commented Dec 3, 2024

I don't get this ci error:

/home/runner/work/agoric-sdk/agoric-sdk/packages/boot/test/fast-usdc/fast-usdc.test.ts
Error:   4:1  error  '@agoric/fast-usdc' should be listed in the project's dependencies. Run 'npm i -S @agoric/fast-usdc' to add it  import/no-extraneous-dependencies

It's complaining about the 1st import from fast-usdc, but it thinks the 2nd one is just fine:

import { configurations } from '@agoric/fast-usdc/src/utils/deploy-config.js';
import { MockCctpTxEvidences } from '@agoric/fast-usdc/test/fixtures.js';

Copy link
Contributor

@samsiegart samsiegart left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

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

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 8ded3d8
Status: ✅  Deploy successful!
Preview URL: https://c6ca88ca.agoric-sdk.pages.dev
Branch Preview URL: https://dc-fu-policy-update.agoric-sdk.pages.dev

View logs

@dckc dckc added the automerge:rebase Automatically rebase updates, then merge label Dec 3, 2024
USDC: await E(USDCissuer).getBrand(),
});
const { feedPolicy } = fromExternalConfig(
config?.options, // just in case config is missing somehow
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't that be an error?

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, but this way, we get a pattern mismatch diagnostic instead of a "can't get .options property of undefined" error.

It's evidently not worth the bother, though. taking it out.

@@ -252,6 +278,16 @@ export default async (homeP, endowments) => {
return JSON.parse(feedPolicy);
};

if (update) {
Copy link
Member

Choose a reason for hiding this comment

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

This isn't really a concern of init fast-usdc, right?

Please have a separate script or include a comment here and at the top explaining why not.

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 could rename it to fast-usdc.builder.js.

I'll try a separate script first to see how much code duplication there is.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

separate script is ~60 lines... about half is duplicated. Not so much that it's worth coupling the two purposes.

@dckc dckc removed the automerge:rebase Automatically rebase updates, then merge label Dec 3, 2024
@dckc dckc requested a review from turadg December 3, 2024 22:46
@dckc dckc added the automerge:rebase Automatically rebase updates, then merge label Dec 3, 2024
@dckc dckc force-pushed the dc-fu-policy-update branch from 631f763 to 9c11a05 Compare December 7, 2024 17:16
@dckc dckc force-pushed the dc-fu-policy-update branch from 9c11a05 to 8ded3d8 Compare December 8, 2024 22:52
@mergify mergify bot merged commit 1645d7e into master Dec 8, 2024
81 checks passed
@mergify mergify bot deleted the dc-fu-policy-update branch December 8, 2024 23:37
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Core-Eval to change policy
3 participants