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: fusdc vstorage #10639

Merged
merged 4 commits into from
Dec 11, 2024
Merged

chore: fusdc vstorage #10639

merged 4 commits into from
Dec 11, 2024

Conversation

0xpatrickdev
Copy link
Member

@0xpatrickdev 0xpatrickdev commented Dec 6, 2024

incidental FUSDC changes

Description

  • publish PoolMetrics to child node
  • publish pool and settlement account addresses to storage
  • add getStaticInfo method to PublicFacet to return orch account addresses

Security Considerations

None

Scaling Considerations

None

Documentation Considerations

None

Testing Considerations

Adds snapshot tests for vstorage and demonstrates usage of getStaticInfo in unit test.

Upgrade Considerations

None, unreleased contract

@0xpatrickdev 0xpatrickdev requested a review from a team as a code owner December 6, 2024 20:19
@0xpatrickdev 0xpatrickdev marked this pull request as draft December 9, 2024 19:46
@0xpatrickdev 0xpatrickdev force-pushed the pc/fusdc-vstorage-updates branch from 4909feb to e3d94c3 Compare December 10, 2024 00:56
Copy link

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

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 475e720
Status: ✅  Deploy successful!
Preview URL: https://2706c7b1.agoric-sdk.pages.dev
Branch Preview URL: https://pc-fusdc-vstorage-updates.agoric-sdk.pages.dev

View logs

@0xpatrickdev 0xpatrickdev changed the base branch from master to dc-start-fast-usdc December 10, 2024 00:56
@0xpatrickdev 0xpatrickdev marked this pull request as ready for review December 10, 2024 00:57
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.

Requested a recorderkit

Also some tests should be affected by these changes. I just realized the tests I wrote in https://github.com/Agoric/agoric-sdk/pull/10633/commits haven't landed. I'll trim that PR to get them in

E(settlementAccount).getAddress(),
]),
);
await publishAddresses(storageNode, {
Copy link
Member

Choose a reason for hiding this comment

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

publishAddresses calling a different publishAddresses is quite confusing. I first thought this should be inlined but I imagine you wanted some documentation and type annotation of the helper.

Usually we would get that with a RecorderKit. Then I thought it's not necessary in this contract because the "on-chain can read same values as off-chain" requirement isn't pertinent to this contract that only works off-chain.

But thinking about it more, it's imaginable that another contract would want a reference to these accounts. E.g. to automate pool participation.

So I think an addressRecorderKit.recorder.write should be used to write these values. A makeRecorderKit is already available in scope.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good feedback, contracts wanting to consume this value crossed my mind over the weekend but I forgot about it when revisiting the changes today.

As an alternative to a recorderKit, what do you think about a getAddresses public facet method? This might be more fitting since we expect the values to be static, or at least append only?

Copy link
Member

Choose a reason for hiding this comment

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

Good point. Since it never changes a RecorderKit is overkill. To emphasize the static nature consider getStaticInfo() or something like that where we could include all static info about the contract

@dckc dckc force-pushed the dc-start-fast-usdc branch from 8874419 to c9ba09e Compare December 10, 2024 06:11
Base automatically changed from dc-start-fast-usdc to master December 10, 2024 06:58
@0xpatrickdev 0xpatrickdev force-pushed the pc/fusdc-vstorage-updates branch from e3d94c3 to bf7b154 Compare December 10, 2024 18:11
@0xpatrickdev 0xpatrickdev force-pushed the pc/fusdc-vstorage-updates branch 2 times, most recently from 21d5049 to 287e5be Compare December 10, 2024 22:57
@0xpatrickdev 0xpatrickdev requested a review from turadg December 10, 2024 23:18
@@ -160,6 +175,23 @@ export const contract = async (zcf, privateArgs, zone, tools) => {
);
});
},
async publishAddresses() {
!baggage.has(ADDRESSES_BAGGAGE_KEY) || Fail`Addresses already published`;
Copy link
Member

Choose a reason for hiding this comment

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

good thinking!

- include `poolMetrics` and account addresses
- ensure contract consumers have access to the contract's orch account addresses
  which are currently only published to vstorage. since these are not expected
  to change, a public facet method seems more suitable than a recorderKit
@0xpatrickdev 0xpatrickdev force-pushed the pc/fusdc-vstorage-updates branch from 287e5be to 475e720 Compare December 11, 2024 00:56
@0xpatrickdev 0xpatrickdev added the automerge:rebase Automatically rebase updates, then merge label Dec 11, 2024
@mergify mergify bot merged commit 5affa34 into master Dec 11, 2024
86 of 91 checks passed
@mergify mergify bot deleted the pc/fusdc-vstorage-updates branch December 11, 2024 01:43
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.

2 participants