feat: add FastLP to vbank #10709

dckc · 2024-12-17T01:33:46Z

Description

add FastLP / ufastlp to vbank

Security Considerations

Adds bankManager to the FastUSDC core eval permit, which introduces an audit burden to see that is used only for the relevant addAsset call.

how to limit excess authority when registering a vbank asset #10722

Scaling Considerations

eliminates up-calls from go to JS on each FastLP balance update

Documentation Considerations

Existing docs suffice; to wit: VBank Assets and Cosmos Bank Balances

Testing Considerations

update tests that relied on getting FastLP balances from published.wallet.${addr}.current.

Upgrade Considerations

unreleased

cloudflare-workers-and-pages · 2024-12-17T01:37:11Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`3815e4c`
Status:	✅ Deploy successful!
Preview URL:	https://e5218bae.agoric-sdk.pages.dev
Branch Preview URL:	https://dc-fastlp-vbank.agoric-sdk.pages.dev

View logs

dckc · 2024-12-17T19:10:59Z

ci shows evidence of the breaking change:

  ✘ [fail]: fast-usdc › lp deposits

  Error {
    message: 'lp has pool shares condition failed after 6 retries.',

Tue, 17 Dec 2024 02:33:48 GMT

turadg · 2024-12-18T00:25:46Z

multichain-testing/tools/purse.ts

-import type { Amount, Brand } from '@agoric/ertp';
-const { fromEntries } = Object;
-
-// @ts-expect-error Type 'Brand' does not satisfy the constraint 'string | number | symbol'


I ran into this recently: https://github.com/Agoric/agoric-sdk/pull/10713/files#diff-a54584fae7da5725f7cb10d222f2eb764b7a9d51256e646ebb8dab4af9a0647fR121

thanks for addressing

turadg · 2024-12-18T00:28:24Z

multichain-testing/test/fast-usdc/fast-usdc.test.ts

@@ -107,23 +110,64 @@ test.after(async t => {
  deleteTestKeys(accounts);
 });

+type VStorageClient = Awaited<ReturnType<typeof commonSetup>>['vstorageClient'];
+const agoricNamesQ = (vsc: VStorageClient) =>


not a blocker but I find these one-off abstractions a little hard to work with. I hope we can design good general abstractions in @agoric/client-utils to invest in. #10713 attempts to replace test calls to VStorageClient with SmartWalletKit, getting typed data for query paths.

If the client-utils abstractions aren't right, let's work on to improve them, so all consumers benefit.

Thanks for #10713; I looked it over and made some comments.

The cycle time in multi-chain testing is extremely high, so I'm not eager to go beyond this 1 test file for this feature.

In general, my habit is to experiment until I see a pattern that works well in at least 2 or 3 cases and then DRY it out.

turadg · 2024-12-18T00:30:18Z

multichain-testing/test/fast-usdc/fast-usdc.test.ts

+  harden({
+    metrics: () =>
+      vsc.queryData(
+        `published.${contractName}.poolMetrics`,


I don't get why this is parameterized for any contractName when it's bound to fastLPQ.

copy/paste left-over.

if I change anything else, I'll remove the parameter. not worth another round of ci on its own, I suggest

turadg · 2024-12-18T00:32:41Z

packages/fast-usdc/src/fast-usdc.start.js

  produceShareIssuer.resolve(shareIssuer);
  produceShareBrand.resolve(shareBrand);
  await publishDisplayInfo(shareBrand, { board, chainStorage });

+  const { denom, issuerName } = ShareAssetInfo;
+  trace('addAsset', denom, shareBrand);
+  await E(bankManager).addAsset(denom, issuerName, issuerName, {


not objecting, but why use a ZCFMint when the start eval is now concerned with the Issuer?

oh, so minting appears synchronous within the contract?

yes. A ZCFMint is the normal Zoe API for a token issued by a contract.

samsiegart · 2024-12-18T04:44:41Z

packages/boot/test/fast-usdc/fast-usdc.test.ts

+  // XXX #10491 should not need to resort to string match on brand
+  t.falsy(
+    purses.find(p => `${p.brand}`.match(/FastLP/)),
+    'FastLP balance not in wallet record',


Is there a way to assert that it is in the wallet's bank in this test context? I might suggest at least saying 'FastLP balance should be in bank, not in wallet record', or maybe put a want in the proposal. Otherwise, looking at this assertion on its own might make it seem like we just don't expect fastlp tokens at all. Maybe it's not necessary at all?

good idea... I think I can do that...

~~Something like this should help with that:~~

Wrote this before reading the code changes, seems you already have this.

const queryClient = makeQueryClient( await useChain('agoric').getRestEndpoint(), ); const { balance } = await queryClient.queryBalance('$ADDR', 'ufastlp')

yeah; there's no rest endpoint in the bootstrap world

Ah getOutboundMessages didn't know about this, neat.

samsiegart

LGTM but deferring approval to other reviewers since I'm not sure about the bankManager stuff in fast-usdc.start.js

samsiegart · 2024-12-18T18:18:46Z

packages/boot/test/fast-usdc/fast-usdc.test.ts

+  // XXX #10491 should not need to resort to string match on brand
+  t.falsy(
+    purses.find(p => `${p.brand}`.match(/FastLP/)),
+    'FastLP balance not in wallet record',


Ah getOutboundMessages didn't know about this, neat.

turadg · 2024-12-18T18:30:53Z

packages/boot/test/fast-usdc/fast-usdc.test.ts


    const { EV } = t.context.runUtils;
    const agoricNames = await EV.vat('bootstrap').consumeItem('agoricNames');
    const board = await EV.vat('bootstrap').consumeItem('board');
    const getBoardAux = async name => {
      const brand = await EV(agoricNames).lookup('brand', name);
      const id = await EV(board).getId(brand);
+      t.is(id || null, lpId);


I had a little trouble making sense of what it's testing. Casting all falsy id to null because lpId will either have a string or null? I can't tell why that's necessary.

please try to make it clearer

yeah; that's goofy.

I fixed it so lpId is definitely a string instead of making the other side maybe null.

The type of t.is() says both sides are the same type.

- prune balancesFromPurses, which was not type-safe - factor out vstorage queries for type safety - add static check on deposit / withdraw proposals - refactor usdcToGive as give.USDC etc. balancesFromPurses() seemed to return a Record<Brand, Amount> but Brands can't be record keys. It "worked" by stringifying the brands. ``` > b = {[Symbol.toStringTag]:'B123'} { [Symbol(Symbol.toStringTag)]: 'B123' } > b.toString() '[object B123]' > fromEntries([[b, 1]]) { '[object B123]': 1 } ```

dckc added the force:integration Force integration tests to run on PR label Dec 17, 2024

dckc force-pushed the dc-fastlp-vbank branch from 0421cec to b34dc50 Compare December 17, 2024 21:55

dckc mentioned this pull request Dec 17, 2024

how to limit excess authority when registering a vbank asset #10722

Open

dckc marked this pull request as ready for review December 17, 2024 22:55

dckc requested a review from a team as a code owner December 17, 2024 22:55

dckc requested review from AgoricTriage, samsiegart and turadg December 17, 2024 22:55

turadg reviewed Dec 18, 2024

View reviewed changes

samsiegart reviewed Dec 18, 2024

View reviewed changes

dckc requested review from samsiegart and turadg December 18, 2024 16:15

samsiegart reviewed Dec 18, 2024

View reviewed changes

turadg approved these changes Dec 18, 2024

View reviewed changes

dckc added the automerge:rebase Automatically rebase updates, then merge label Dec 18, 2024

dckc added 5 commits December 18, 2024 20:19

feat(fast-usdc): add FastLP/ufastlp to vbank

ae1963e

test(boot): FastLP/ufastlp is in published.agoricNames.vbankAsset

b7aa5f0

test(boot): FastLP balance not in wallet record

2ba4a95

chore: walletDriver supports getAddress()

3815e4c

dckc force-pushed the dc-fastlp-vbank branch from a5dff2f to 3815e4c Compare December 18, 2024 20:19

mergify bot merged commit c1d2b71 into master Dec 18, 2024
81 checks passed

mergify bot deleted the dc-fastlp-vbank branch December 18, 2024 21:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add FastLP to vbank #10709

feat: add FastLP to vbank #10709

dckc commented Dec 17, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Dec 17, 2024 •

edited

Loading

dckc commented Dec 17, 2024

turadg Dec 18, 2024

turadg Dec 18, 2024

dckc Dec 18, 2024

turadg Dec 18, 2024

dckc Dec 18, 2024

turadg Dec 18, 2024 •

edited

Loading

dckc Dec 18, 2024

samsiegart Dec 18, 2024

dckc Dec 18, 2024

0xpatrickdev Dec 18, 2024 •

edited

Loading

dckc Dec 18, 2024

samsiegart Dec 18, 2024

samsiegart left a comment

samsiegart Dec 18, 2024

turadg Dec 18, 2024

dckc Dec 18, 2024 •

edited

Loading

feat: add FastLP to vbank #10709

feat: add FastLP to vbank #10709

Conversation

dckc commented Dec 17, 2024 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Dec 17, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

dckc commented Dec 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xpatrickdev Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samsiegart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckc Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

dckc commented Dec 17, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Dec 17, 2024 •

edited

Loading

turadg Dec 18, 2024 •

edited

Loading

0xpatrickdev Dec 18, 2024 •

edited

Loading

dckc Dec 18, 2024 •

edited

Loading