-
Notifications
You must be signed in to change notification settings - Fork 207
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
Fast USDC CLI: Transfer #10437
Fast USDC CLI: Transfer #10437
Conversation
Deploying agoric-sdk with Cloudflare Pages
|
cdc9d32
to
04f8e3f
Compare
Is there some particular reason to land this without such a test? |
Are you blocked on #10441? It would be better to merge this sooner than later so that we could make that PR fit the structure of this one. If it's not a blocker, I can add the tests if this looks good to you so far. I mostly wanted to make sure my approach was agreeable before doing all that extra work. For example, removing mock-fs was significant feedback, and I might've been inclined to use a monkey-patching approach on the rest of the stuff otherwise. I've gone ahead and removed mock-fs though, and used file objects instead as discussed, so PTAL @dckc |
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.
2 more suggestions similar to file objects instead of filenames.
No; #10441 is just for discussion. It's a bunch of suggestions about an approach for testing and such.
Sure, a lot of it looks good. |
14de6d4
to
971aa7b
Compare
export const makeFile = ( | ||
/** @type {string} */ path, | ||
/** @type {readAsync} */ readFile, | ||
/** @type {writeAsync} */ writeFile, | ||
/** @type {mkdirSync} */ mkdir, | ||
/** @type {existsSync} */ pathExists, | ||
) => { |
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.
suggestion: With more than 3 args, naming them tends to help. It works nicely with Pick<typeof fs, 'readFile' | 'writeFile' | ...>
too.
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 args have gotten long in quite a few places because of the need for dependency injection, so I'll look through this last thing before merging to see if I can clean it up some.
packages/fast-usdc/src/util/noble.js
Outdated
const clientWithSigner = await SigningStargateClient.connectWithSigner( | ||
nobleRpc, |
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.
nobleRpc
should be an RPC client object rather than a string. Something like this should work, based on some endo playground experience:
Use makeTendermintRpcClient(url, fetch) from casting and the rest comes from cosmjs:
import { Tendermint34Client } from '@cosmjs/tendermint-rpc';
...
const cometClient = await Tendermint34Client.create(rpcClient);
const clientWithSigner = await SigningStargateClient.createWithSigner(
cometClient,
...
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.
Why though? It adds more dependencies and lines of code, while the SigningStargateClient
interface supports a string
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 adds more dependencies
It adds lines in package.json
maybe, but the dependencies are already there, indirectly.
SigningStargateClient
interface supports a string
yes, but that string separates designation from authority. If we pass a string, SigningStargateClient
is going to use ambient access to the network - fetch
or the like. How would we test registerFwdAccount
? Perhaps there are tools that use global mutable state to override fetch
. That sort of spooky action-at-a-distance is difficult to manage at scale. And it means registerFwdAccount
doesn't support patterns of reuse such as passing in an RPCClient with built-in rate-limiting.
const makeConfigFile = () => | ||
makeFile(getConfigPath(), readFile, writeFile, mkdir, exists); |
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.
most of the code doesn't seem to need write access.
Consider grabbing makeFileRW and makeFileRd from synthetic-chain. make a RW and then call .readOnly()
; only pass the RW to the things that need 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.
we can do that later. not critical.
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.
Ack
Added test coverage for the full transfer flow now |
d60b4da
to
bc4554d
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.
It mostly looks great, but...
It looks like it gives a successful exit code (0) on failure, which makes it unsuitable as an integration testing tool. See silent failure comments.
Consider the suggestions entirely at your discretion; feel free to ignore entirely, decline, adopt, adapt, etc.
t.deepEqual(signerMock.getSigned(), [ | ||
nobleSignerAddress, | ||
[ | ||
{ | ||
typeUrl: '/noble.forwarding.v1.MsgRegisterAccount', | ||
value: { | ||
channel: nobleToAgoricChannel, | ||
recipient: `${agoricSettlementAccount}+${destination}`, | ||
signer: nobleSignerAddress, | ||
}, | ||
}, | ||
], | ||
{ | ||
amount: [ | ||
{ | ||
amount: '20000', | ||
denom: 'uusdc', | ||
}, | ||
], | ||
gas: '200000', | ||
}, | ||
'Register Account Transaction', | ||
]); | ||
}); |
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 is a pretty important external interface. Consider using t.snapshot()
rather than t.deepEqual()
; the snapshot markdown files are particularly useful docs. Plus, they're low maintenance (just run yarn test -u
and check the diffs).
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.
Done
{ chainName: 'agoric', rpcAddrs: [config.agoricRpc] }, | ||
); | ||
const agoricAddr = await queryFastUSDCLocalChainAccount(vstorage, out); | ||
const appendedAddr = `${agoricAddr}+${destination}`; |
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 +
syntax isn't defined by this module, is it? It seems like we should be able to import a function that combines these two.
I suggest making a local function and marking it TODO: update syntax to sync with other components
or something.
Are we even still using +
syntax? In something linked from Fast USDC Eng Design / Sketch, I see:
❓ How should the parameters be separated from the address?
✓ Use "?" separator
✓ How should the parameters be encoded?
✓ Use URL query param encoding
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.
Fixed, after discussion we're going with ?EUD=
, good catch.
t.deepEqual(mockEthProvider.getTxnArgs()[0], [ | ||
'0xf8a4800180941c7d4b196cb0c7b01d743fbc6116a902379c723880b844095ea7b30000000000000000000000009f3b8679c73c2fef8b59b4f3444d4e156fb70aa50000000000000000000000000000000000000000000000000000000008f0d18082011aa0b2d87eeb1cb36243f95662739e2a7bd4bddc2b8afe189ac4848ec71cc314335ba068136695c644f69474e2e30ea7059f9b380fbb1a09beb3580f73d3ea349912ab', | ||
]); |
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 wonder how to review that. I suppose we'll eventually do integration testing, and we'll find out real quick if it's not right at that point.
Meanwhile, I did try to puzzle it out a little, and this form might be slightly more legible. Dunno whether it's really useful, but I'll share it rather than throw it away:
t.deepEqual(mockEthProvider.getTxnArgs()[0], [ | |
'0xf8a4800180941c7d4b196cb0c7b01d743fbc6116a902379c723880b844095ea7b30000000000000000000000009f3b8679c73c2fef8b59b4f3444d4e156fb70aa50000000000000000000000000000000000000000000000000000000008f0d18082011aa0b2d87eeb1cb36243f95662739e2a7bd4bddc2b8afe189ac4848ec71cc314335ba068136695c644f69474e2e30ea7059f9b380fbb1a09beb3580f73d3ea349912ab', | |
]); | |
t.deepEqual(ethers.decodeRlp(mockEthProvider.getTxnArgs()[0]), [ | |
'0x', | |
'0x01', | |
'0x', | |
'0x1c7d4b196cb0c7b01d743fbc6116a902379c7238', | |
'0x', | |
'0x095ea7b30000000000000000000000009f3b8679c73c2fef8b59b4f3444d4e156fb70aa50000000000000000000000000000000000000000000000000000000008f0d180', | |
'0x011a', | |
'0xb2d87eeb1cb36243f95662739e2a7bd4bddc2b8afe189ac4848ec71cc314335b', | |
'0x68136695c644f69474e2e30ea7059f9b380fbb1a09beb3580f73d3ea349912ab', | |
]); |
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.
Yea I'm not really sure either. Just left a comment because I found a website that can decode it. But I spent a little bit trying to do it myself with ethers
and couldn't succeed.
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.
cool. the comment is quite nice.
t.is(file.path, path); | ||
}); | ||
|
||
test('reads the file contents', async 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.
this file of tests for makeFile
seems like overkill. Sorry if I suggested otherwise. I suppose they don't hurt much, other than maintenance.
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.
Np
configHelpers = configLib, | ||
transferHelpers = transferLib, |
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 it useful to inject these 2 any more? Don't we just inject fetch
and makeSigner
and such?
note: ok... I get it... we mock transferHelpers
to check that initProgram
calls transferHelpers
as expected and then unit test transferHelpers
such as .transfer
. fair enough.
/** @typedef {import('../util/file').file} file */ | ||
/** @typedef {import('@agoric/client-utils').VStorage} VStorage */ | ||
/** @typedef {import('@cosmjs/stargate').SigningStargateClient} SigningStargateClient */ | ||
/** @typedef {import('ethers').ethers.JsonRpcProvider} ethProvider */ |
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.
suggestion: use @import
rather than @typedef
. @typedef
loses docstrings.
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.
Ooh TIL... done.
const transfer = async ( | ||
/** @type {file} */ configFile, | ||
/** @type {string} */ amount, | ||
/** @type {string} */ destination, |
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 rest of agoric-sdk uses @param
. This is more concise in a way, so I don't mind too much. It's also more like typescript, which we seem to be headed toward. So maybe it's better.
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.
Ack, if you don't feel strongly I'll leave as is
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.
LGTM. We might have the whole package on .ts
soon: #10480
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.
PTAL
/** @typedef {import('../util/file').file} file */ | ||
/** @typedef {import('@agoric/client-utils').VStorage} VStorage */ | ||
/** @typedef {import('@cosmjs/stargate').SigningStargateClient} SigningStargateClient */ | ||
/** @typedef {import('ethers').ethers.JsonRpcProvider} ethProvider */ |
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.
Ooh TIL... done.
const transfer = async ( | ||
/** @type {file} */ configFile, | ||
/** @type {string} */ amount, | ||
/** @type {string} */ destination, |
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.
Ack, if you don't feel strongly I'll leave as is
{ chainName: 'agoric', rpcAddrs: [config.agoricRpc] }, | ||
); | ||
const agoricAddr = await queryFastUSDCLocalChainAccount(vstorage, out); | ||
const appendedAddr = `${agoricAddr}+${destination}`; |
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.
Fixed, after discussion we're going with ?EUD=
, good catch.
out.error( | ||
`Error noble registering forwarding account for ${appendedAddr} on channel ${config.nobleToAgoricChannel}`, | ||
); | ||
return; |
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.
No you're right, it shouldn't be considered a success condition. I just went back and made sure all these fatal errors actually throw.
t.deepEqual(signerMock.getSigned(), [ | ||
nobleSignerAddress, | ||
[ | ||
{ | ||
typeUrl: '/noble.forwarding.v1.MsgRegisterAccount', | ||
value: { | ||
channel: nobleToAgoricChannel, | ||
recipient: `${agoricSettlementAccount}+${destination}`, | ||
signer: nobleSignerAddress, | ||
}, | ||
}, | ||
], | ||
{ | ||
amount: [ | ||
{ | ||
amount: '20000', | ||
denom: 'uusdc', | ||
}, | ||
], | ||
gas: '200000', | ||
}, | ||
'Register Account Transaction', | ||
]); | ||
}); |
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.
Done
t.deepEqual(mockEthProvider.getTxnArgs()[0], [ | ||
'0xf8a4800180941c7d4b196cb0c7b01d743fbc6116a902379c723880b844095ea7b30000000000000000000000009f3b8679c73c2fef8b59b4f3444d4e156fb70aa50000000000000000000000000000000000000000000000000000000008f0d18082011aa0b2d87eeb1cb36243f95662739e2a7bd4bddc2b8afe189ac4848ec71cc314335ba068136695c644f69474e2e30ea7059f9b380fbb1a09beb3580f73d3ea349912ab', | ||
]); |
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.
Yea I'm not really sure either. Just left a comment because I found a website that can decode it. But I spent a little bit trying to do it myself with ethers
and couldn't succeed.
t.is(file.path, path); | ||
}); | ||
|
||
test('reads the file contents', async 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.
Np
@@ -32,7 +32,7 @@ const transfer = async ( | |||
{ chainName: 'agoric', rpcAddrs: [config.agoricRpc] }, | |||
); | |||
const agoricAddr = await queryFastUSDCLocalChainAccount(vstorage, out); | |||
const appendedAddr = `${agoricAddr}+${destination}`; | |||
const appendedAddr = `${agoricAddr}?EUD=${destination}`; |
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 implementation assumes destination
doesn't need URL encoding. I suppose that's true for any correct bech32 addresses. But I'm not sure we validate that destination
is a correct bech32 address.
packages/fast-usdc/src/utils/address.js
has some addressTools
. I wonder if a function there to combine these is worthwhile. I'm OK postponing until the need is more evident.
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 had to fix this anyway. It will now encode all of appendedAddr
, which I think should work regardless.
/** | ||
* https://github.com/noble-assets/forwarding/blob/9d7657a/proto/noble/forwarding/v1/query.proto | ||
* v2.0.0 10 Nov 2024 | ||
*/ |
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 meant a docstring on queryForwardingAccount
. sorry i wasn't clear. not critical.
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 would but then it makes me pull all the args into the docstring as well, quite annoying
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.
oh... if using @type
rather than @param
means no prose docstrings, that's going to be a problem real soon.
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.
Logs from a successful run after updating the URL encoding (ignore the insufficient funds thing, that's just because my eth account was empty. Also, the "agoric1234asdadfga" address was stubbed before running):
|
501ee86
to
98d08f3
Compare
98d08f3
to
d46cefd
Compare
refs #10339
Description
config
command for setting all the required variables relating to IBC channels, mnemonics, rpc endpoints, etc.transfer
command for simulating the Noble Express frontend. It accepts an end user cosmos address destination, and a USDC amount as args. TODO: It should query the agoric Fast USDC LCA address, but for now it just stubs a hardcoded address there, because I'm not sure what the query should look like.Documentation Considerations
Tried to make the
help
command as self-documenting as possibleTesting Considerations
Manually tested on testnets with testnet tokens that:
I put the config used for that test run in
demo/testnet
for reference.Added unit tests for all the config stuff, but didn't unit test "transfer" yet. Maybe e2e tests would be better for that part...
Upgrade Considerations
The CLI is a client program and doesn't need to be upgraded in production.