-
Notifications
You must be signed in to change notification settings - Fork 215
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
fix: agops: no I/O (errors) until command action #7299
Conversation
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.
Some cleanup suggested by not required. There are lint violations but I'll leave that between you and CI.
@@ -6,8 +6,6 @@ import { Command } from 'commander'; | |||
import { makeRpcUtils, storageHelper } from '../lib/rpc.js'; | |||
import { outputExecuteOfferAction } from '../lib/wallet.js'; | |||
|
|||
const { vstorage, fromBoard, agoricNames } = await makeRpcUtils({ fetch }); |
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.
ah yes. I think this was made one in rpc.js
before but that was bad so it was factored it into a maker, but then didn't go the distance to only make where used.
} | ||
return instance; | ||
const rpcTools = async () => { | ||
const { agoricNames, fromBoard, vstorage } = await makeRpcUtils({ fetch }); |
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.
consider letting the object pass through,
const { agoricNames, fromBoard, vstorage } = await makeRpcUtils({ fetch }); | |
const utils = await makeRpcUtils({ fetch }); |
and then,
const instance = utils.agoricNames.instance[name];
…
return {...utils, lookupPriceAggregatorInstance};
} | ||
return instance; | ||
const rpcTools = async () => { | ||
const { vstorage, fromBoard, agoricNames } = await makeRpcUtils({ fetch }); |
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.
ditto with comment above. if we do this often maybe worth another argument to makeRpcUtils
that takes a maker for more utils.
const rpcTools = () => return makeRpcUtils(
{fetch},
base => {
lookupPsmInstance: ([minted, anchor]) => {
const name = `psm-${minted}-${anchor}`;
const instance = base.agoricNames.instance[name];
if (!instance) {
logger.debug('known instances:', agoricNames.instance);
throw new Error(`Unknown instance ${name}`);
}
return instance;
}
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 usually go for the objects as closures pattern...
const makeThing = powers => {
...
return harden({ method1, method2, ... });l
}
... and think about POLA while I'm at it. The bundles of rights that come to mind are:
- an RPC client, including vstorage. it would know about network-config and ideally do failover
- such a client that also has
fromBoard
state and knows about board marshalling - keyring access: just map names to addresses, no private key access
- keyring+RPC: a sign-and-broadcast tool
refs: #6930
factoring out of #7256
Description
Each of
commands/oracle.js
,commands/ec.js
etc. was callingmakeRpcUtils
on module import. Not only did this slow things down, it made catching I/O errors in theagops inter
command infeasible.Scaling Considerations
about 20 fewer RPC requests per
agops
command.Security, Documentation Considerations
Not much... perhaps nicer diagnostics for
agops inter
is documentation.Testing Considerations
no change in the (lack of) automated testing