-
Notifications
You must be signed in to change notification settings - Fork 214
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
chore: fusdc vstorage #10639
Conversation
4909feb
to
e3d94c3
Compare
Deploying agoric-sdk with Cloudflare Pages
|
There was a problem hiding this 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, { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
8874419
to
c9ba09e
Compare
e3d94c3
to
bf7b154
Compare
21d5049
to
287e5be
Compare
@@ -160,6 +175,23 @@ export const contract = async (zcf, privateArgs, zone, tools) => { | |||
); | |||
}); | |||
}, | |||
async publishAddresses() { | |||
!baggage.has(ADDRESSES_BAGGAGE_KEY) || Fail`Addresses already published`; |
There was a problem hiding this comment.
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
287e5be
to
475e720
Compare
incidental FUSDC changes
Description
PoolMetrics
to child nodegetStaticInfo
method to PublicFacet to return orch account addressesSecurity 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