-
Notifications
You must be signed in to change notification settings - Fork 217
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 of replacing fast-usdc operators #10834
Conversation
Deploying agoric-sdk with Cloudflare Pages
|
a0c26df
to
81558ca
Compare
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.
The tests show that this does what we need.
The @type {CoreEvalBuilder}
vs. @satisfies {CoreEvalBuilder}
is hiding quite a few type errors. I expect you'll want to clean that up, but I'm OK with postponing it indefinitely.
|
||
{ | ||
// no invitation before | ||
const curr = await vstorageKit.readPublished(addr); |
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.
The name addr
threw me off; I assumed it started with agoric1...
and did a double-take when I got to here.
not critical.
await writeCoreEval('add-operators', utils => | ||
defaultProposalBuilder(utils, config), | ||
); | ||
}; |
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.
80 lines of almost completely boilerplate code. This gets boring.
The platform is fixed. The platform is fixed. The platform is fixed...
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.
yeah, I felt like I was fighting the APIs a bit here. another day.
const trace = makeTracer('FUSD-AddOperators', true); | ||
|
||
/** @type {TypedPattern<FastUSDCConfig>} */ | ||
export const FastUSDCConfigShape = M.splitRecord({ |
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.
Is this duplicated? Should it move to ./type-guards.js
?
* instance: PromiseSpaceOf<{ fastUsdc: Instance<FastUsdcSF> }>; | ||
* issuer: PromiseSpaceOf<{ FastLP: Issuer }>; | ||
* brand: PromiseSpaceOf<{ FastLP: Brand }>; | ||
* }} FastUSDCCorePowers |
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.
FastUSDCCorePowers
looks familiar too. move to types.js
?
USDC: Far('USDC Brand'), | ||
}); | ||
|
||
/** @type {CoreEvalBuilder} */ |
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.
/** @type {CoreEvalBuilder} */ | |
/** @satisfies {CoreEvalBuilder} */ |
Using @type
hides type errors around config
.
Maybe not critical, but probably worth fixing.
getManifestCall: [ | ||
getManifestForAddOperators.name, | ||
{ | ||
options: toExternalConfig(config, crossVatContext), |
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.
You could skip toExternalConfig
since this is JSON-happy data.
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.
understood. I try to keep the differences to line deletion as much as possible, for easier reconciliation
/** @type {CoreEvalBuilder} */ | ||
export const defaultProposalBuilder = async ( | ||
powers, | ||
/** @type {FastUSDCConfig} */ config, |
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.
/** @type {FastUSDCConfig} */ config, | |
/** @type {{ oracles: Record<string, string> }} */ config, |
* restoreRef: (b: ERef<ManifestBundleRef>) => Promise<Installation>; | ||
* }} utils | ||
* @param {{ | ||
* options: LegibleCapData<FastUSDCConfig>; |
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.
The builder isn't providing a whole FastUSDCConfig
.
See @satisfies
vs @type
earlier.
// widely shared: name services | ||
agoricNames: true, | ||
namesByAddress: true, | ||
board: true, |
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.
board
doesn't seem to be used.
const toWatch = await E(creatorFacet).makeOperatorInvitation(address); | ||
|
||
const amt = await E(depositFacet).receive(toWatch); | ||
trace('sent', amt, 'to', name); |
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.
The exo's use a default arg for trace
. The idea appeals to me here to let the caller keep one trace sequence. not sure how worthwhile it is.
81558ca
to
d84d27e
Compare
This pull request has been removed from the queue for the following reason: 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: |
sourceSpec: '@agoric/fast-usdc/src/fast-usdc.start.js', | ||
sourceSpec: '@agoric/fast-usdc/src/start-fast-usdc.core.js', |
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.
I didn't notice renaming before the extension until now...
I quite like having fast-usdc.contract.js
and the core-eval code to start it next to each other. So I'd prefer fast-usd-start.core.js
.
OTOH, I hope that in due course, we mostly avoid custom code for starting contracts. So I can live with whatever.
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.
Thanks for the flexibility. Let's devise together some consistent naming pattern and then apply it across the codebase. It's a non-breaking change
86cc97d
to
da7c727
Compare
This pull request has been removed from the queue for the following reason: 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: |
closes: #10754
Description
Adds a test for adding new operators after removing
Security Considerations
Scaling Considerations
Documentation Considerations
Testing Considerations
Upgrade Considerations