-
Notifications
You must be signed in to change notification settings - Fork 212
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): operator attest cli command #10610
Conversation
I expect to squash at least some of these commits. I iterated on the design a bit and I don't want to throw away the exploration until there's more buy-in on the details. |
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.
The changes requested are mostly for the test helpers, but also consider the env
feedback and broadcasting the txn
|
||
const marshalData = makeMarshal(_v => assert.fail('data only')); | ||
|
||
const mockStream = <T extends Writable>(buf: string[]): T => |
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.
Could you please put this in testing/mocks
? It's better than the existing mockOut
thing so should be used going forward.
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.
We don't export testing
from packages. Mocks are often useful outside a package so consider fast-usdc/tools/testingMocks.js
or somesuch.
import { makeMarshal } from '@endo/marshal'; | ||
import type { Writable } from 'node:stream'; | ||
|
||
export const flags = ( |
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: adding this to a new testing/helpers.ts
(or similar) file.
proposal: {}, | ||
}; | ||
|
||
outputActionAndHint( |
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 guess this is okay for now, would be great if it just went ahead and did the tx. There's an example here of it broadcasting a noble tx: https://github.com/Agoric/agoric-sdk/blob/master/packages/fast-usdc/src/util/noble.js#L46. This feedback is optional.
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.
It would be great but there's also the matter of managing key material which is already in agd.
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.
That's understandable, it's reasonable to assume this will be run in an environment with agd already. For noble, I didn't want to require nobled, so I added it.
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 do hope @agoric/client-utils
will sort of standardize on the signAndBroadcast(addr, [msg], ...)
API. It's an interface that can be implemented using either agd
or keplr as well as cosmjs.
something like:
makeAgd({execFile}).withOpts({keyringBackend: 'test'}).account('gov1').signAndBroadcast(...)
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 idea. I've added that to
env: process.env, | ||
fetch, | ||
}); | ||
const networkConfig = await fetchEnvNetworkConfig({ env, 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.
I guess env
was introduced at some point. It's not great to have both env
and a config file that can conflict. I'd ask that we at least make
agoric-sdk/packages/fast-usdc/src/cli/transfer.js
Lines 30 to 32 in ef82fbf
vstorage ||= makeVStorage( | |
{ fetch }, | |
{ chainName: 'agoric', rpcAddrs: [config.agoricRpc] }, |
fetchEnvNetworkConfig
also, and remove agoricRpc from the config file. Also, not sure how well documented the env
thing is, might be worth adding to a README or help command somehow, even if the only intended user is "us".
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 guess env was introduced at some point.
Reading from AGORIC_NET
from env has been in the agoric
command for years. #10570 gave getNetworkConfig
a more accurate name of fetchEnvNetworkConfig
.
use fetchEnvNetworkConfig also
I support this suggestion and the rationale.
Also, not sure how well documented the env thing is, might be worth adding to a README or help command somehow, even if the only intended user is "us".
The intended user of fetchEnvNetworkConfig
is anyone developing a CLI tool that wants to use AGORIC_NET
env var to pick the network. +1 to adding to this CLI tool that it observes AGORIC_NET
env var.
If this PR keeps the "closes" line then I think the help should be included. Alternately it can be saved for another PR and the issue left open.
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.
Ok. I migrated transfer
to use AGORIC_NET and added an epilogue to the help text for all commands:
$ yarn -s fast-usdc transfer --help
Usage: fast-usdc transfer [options] <amount> <dest>
Transfer USDC from Ethereum/L2 to Cosmos via Fast USDC
Arguments:
amount Amount to transfer denominated in uusdc
dest Destination address in Cosmos
Options:
-h, --help display help for command
Agoric test networks provide configuration info at, for example,
https://devnet.agoric.net/network-config
To use RPC endpoints from such a configuration, use:
export AGORIC_NET=devnet
Use AGORIC_NET=local or leave it unset to use localhost and chain id agoriclocal.
@@ -1,3 +1,5 @@ | |||
/* eslint-env node */ | |||
/* global globalThis */ |
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.
would s/globalThis/global
work?
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.
My impression is that globalThis
is more portable than global
:
The
globalThis
property provides a standard way of accessing the global this value (and hence the global object itself) across environments. -- globalThis - JavaScript | MDN
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.
This runs only in a Node env anyway. There's no scenario in which cli.js
runs in a browser.
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 guess I'm not familiar with when to use global
. I don't recall using it. Checking nodejs docs, I see:
Stability: 3 - Legacy. Use globalThis instead.
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.
TIL
I wonder why eslint-env node
doesn't include it
@@ -1,18 +1,42 @@ | |||
/* eslint-env node */ |
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 maintain that CLI modules that are never exported from the package should have access to Node env. The work is done so I won't request that it be reverted.
I see further down that you use the params to mock for testing. I'm sympathetic to that, though I wonder if we'd be better off with testing the actual CLI using execa.
using a LegibleCapData blob arg
closes: #10567
Description
Fill in the operator
attest
action and test it.Security Considerations
none; it writes to stdout
Scaling / Upgrade Considerations
none
Documentation Considerations
It's quite minimalistic. It requires a
--previousOfferId
without providing afind-continuing-id
sub-command.It doesn't handle sign/broadcast nor reporting the outcome of the offer. I suppose
agops perf satisfaction
is available for that; I just hope don't end up relying onagops
in production use.Testing Considerations
One happy-path test, using injected io, to check that the bridgeAction that it writes is as expected.