chore: `advancer` validates `settlementAccount` address #10723

0xpatrickdev · 2024-12-17T23:37:08Z

Description

Primary aim is to validate evidence reported to FUSDC contains the expected settlementAccount address. This supports C8 - Contract MUST be able to initialize settlement process when Noble mints USDC.

To facilitate this change:

add makeTestAddress helper in @agoric/orchestration/tools that returns a valid bech32 address
- since encodeAddressHook throws when presented agoric1FakeLCAAddress
bootstrap's VLOCALCHAIN_ALLOCATE_ADDRESS now returns a valid (mocked) bech32 address
makeFakeLocalchainBridge accepts makeAddressFn

Security Considerations

Hardens FUSDC implementation.

Scaling Considerations

None

Documentation Considerations

None

Testing Considerations

Includes new tests (see advancer.test.ts) and updates existing tests

Upgrade Considerations

None, unreleased code

cloudflare-workers-and-pages · 2024-12-17T23:55:27Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`e06f12a`
Status:	✅ Deploy successful!
Preview URL:	https://43cc6152.agoric-sdk.pages.dev
Branch Preview URL:	https://pc-validate-settlement-accou.agoric-sdk.pages.dev

View logs

turadg · 2024-12-18T19:42:33Z

packages/orchestration/test/tools/make-test-address.test.ts

+  t.is(makeTestAddress(), 'agoric1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqp7zqht');
+  t.is(makeTestAddress(1), 'agoric1qyqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqc09z0g');


consider making the index human readable in the address and/or allowing the caller to add a string to give meaning to the address. That makes it easier to follow in logs what is happening.

Maybe it's a different function. I could see it coming in a separate PR.

very open to a "vanity address" that's more readable. A constraint on inserting a readable string is the checksum encodeAddressHook performs.

turadg · 2024-12-18T19:50:53Z

packages/vats/tools/fake-bridge.js

+export const makeFakeLocalchainBridge = (
+  zone,
+  onToBridge = () => {},
+  makeAddressFn,


Suggested change

makeAddressFn,

makeAddressFn = index => `${LOCALCHAIN_DEFAULT_ADDRESS}${index || ''}`

turadg · 2024-12-18T19:53:25Z

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

@@ -233,7 +234,13 @@ test.serial('makes usdc advance', async t => {
  });
  await eventLoopIteration();

-  const evidence = MockCctpTxEvidences.AGORIC_PLUS_OSMO();
+  const accountsData = storage.data.get('published.fastUsdc');
+  const { settlementAccount } = JSON.parse(JSON.parse(accountsData!).values[0]);


I think storage.getDeserialized would help here

TIL. Not here, but elsewhere in this file it seems we could use something similar for feeConfig.

turadg · 2024-12-18T19:53:38Z

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

+  const { settlementAccount } = JSON.parse(JSON.parse(accountsData!).values[0]);
+  const evidence = MockCctpTxEvidences.AGORIC_PLUS_OSMO(
+    encodeAddressHook(settlementAccount, {
+      EUD: 'osmo183dejcnmkka5dzcu9xw6mywq0p2m5peks28men',


magic string should be a const

turadg · 2024-12-18T20:00:29Z

packages/fast-usdc/src/exos/advancer.js

            const { recipientAddress } = evidence.aux;
            const decoded = decodeAddressHook(recipientAddress);
            mustMatch(decoded, AddressHookShape);
+            if (!settlementAddress?.value) {
+              throw Fail`⚠️ No 'settlementAddress'. must call 'publishAddresses' first.`;


publishAddresses is outside the concern of this module. It only knows about setSettlementAddress

since handleTransactionEvent will fail if setSettlementAddress hasn't been called, maybe the contract shouldn't start the watcher until it has.

but… why not require settlementAddress when making the exo? It's known at that time. Then you can rely on it being set and not have to handle mutations.

We only have the account object at the time the exo is created, not the address one. Agree it would be better to require it at make time. If awaiting getAddress() doesn't break the upgrade test, I'm up for it.

I gave it a try in ac84a97. Let's see how force:integration does with it.

Another reason to make the change is POLA… advancer should not be able to access funds from the settlementAccount.

turadg · 2024-12-18T20:02:50Z

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

@@ -543,7 +549,7 @@ test.serial('STORY01: advancing happy path for 100 USDC', async t => {
    bridges: { snapshot, since },
    mint,
  } = t.context;
-  const cust1 = makeCustomer('Carl', cctp, txPub.publisher, feeConfig);
+  const cust1 = makeCustomer('Carl', cctp, txPub.publisher, feeConfig, storage);


Suggested change

const cust1 = makeCustomer('Carl', cctp, txPub.publisher, feeConfig, storage);

const cust1 = makeCustomer('Carl', cctp, txPub.publisher, feeConfig);

all the methods of cust1 already have access to context and thus storage.

t is passed in with every call. That could be refactored but is out of scope

turadg · 2024-12-18T20:03:18Z

packages/fast-usdc/test/fixtures.ts

+export const settlementAddress: ChainAddress = harden({
+  chainId: 'agoric-3',
+  encoding: 'bech32' as const,
+  value: 'agoric16kv2g7snfc4q24vg3pjdlnnqgngtjpwtetd2h689nz09lcklvh5s8u37ek',


random? if it's not encoding any data let's put something descriptive in it

These was extracted from existing code on master. I don't think it's encoding anything - it's supposed to represent an LCA. Maybe better to use makeTestAddress().

Since this is in a fixtures module I'd propose a fixed value. makeTestAddress values could collide. We might also consider having a global counter so each call produces a unique address. That will help prevent using magic strings: when we need values to match put them in a constant.

- returns valid bech32 address for tests

- instead of `agoric1fakeLCAAddress`, `agoric1fakeLCAAddress1` - to support `encodeAddressHook` which relies on valid bech32

turadg

I made the requested changes as agreed. Approving.

0xpatrickdev · 2024-12-18T23:23:01Z

packages/fast-usdc/src/exos/advancer.js

@@ -271,6 +277,7 @@ export const prepareAdvancerKit = (
        borrowerFacet: M.remotable(),
        poolAccount: M.remotable(),
        intermediateRecipient: M.opt(ChainAddressShape),
+        settlementAddress: M.opt(ChainAddressShape),


minor: should remove M.opt

Thanks for looking it over again. e49ad00 coming in another PR

0xpatrickdev force-pushed the pc/validate-settlement-account branch from d7089f7 to 2ab0286 Compare December 17, 2024 23:55

turadg mentioned this pull request Dec 18, 2024

transaction feed validates settlement account #10720

Closed

0xpatrickdev force-pushed the pc/validate-settlement-account branch from 28bef35 to 0087d71 Compare December 18, 2024 01:16

0xpatrickdev added the force:integration Force integration tests to run on PR label Dec 18, 2024

0xpatrickdev force-pushed the pc/validate-settlement-account branch from 0087d71 to d52f76d Compare December 18, 2024 19:28

0xpatrickdev marked this pull request as ready for review December 18, 2024 19:34

0xpatrickdev requested a review from a team as a code owner December 18, 2024 19:34

0xpatrickdev requested review from AgoricTriage, turadg and dckc December 18, 2024 19:34

0xpatrickdev force-pushed the pc/validate-settlement-account branch from d52f76d to c8e0529 Compare December 18, 2024 19:42

turadg reviewed Dec 18, 2024

View reviewed changes

turadg self-assigned this Dec 18, 2024

0xpatrickdev added 4 commits December 18, 2024 14:04

tools: makeTestAddress helper

6f49369

- returns valid bech32 address for tests

chore: mock VLOCALCHAIN_ALLOCATE_ADDRESS handler returns valid bech32

45eeee0

- instead of `agoric1fakeLCAAddress`, `agoric1fakeLCAAddress1` - to support `encodeAddressHook` which relies on valid bech32

chore: makeFakeLocalchainBridge accepts makeAddressFn

5ad59bd

chore: advancer validates settlementAddress

e06f12a

turadg force-pushed the pc/validate-settlement-account branch from ac84a97 to e06f12a Compare December 18, 2024 22:13

turadg approved these changes Dec 18, 2024

View reviewed changes

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

mergify bot merged commit 7b7ceb9 into master Dec 18, 2024
86 of 95 checks passed

mergify bot deleted the pc/validate-settlement-account branch December 18, 2024 23:03

0xpatrickdev commented Dec 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: `advancer` validates `settlementAccount` address #10723

chore: `advancer` validates `settlementAccount` address #10723

0xpatrickdev commented Dec 17, 2024 •

edited

Loading

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

edited

Loading

turadg Dec 18, 2024

0xpatrickdev Dec 18, 2024

turadg Dec 18, 2024

turadg Dec 18, 2024

0xpatrickdev Dec 18, 2024

turadg Dec 18, 2024

turadg Dec 18, 2024

0xpatrickdev Dec 18, 2024 •

edited

Loading

turadg Dec 18, 2024

turadg Dec 18, 2024

turadg Dec 18, 2024

0xpatrickdev Dec 18, 2024

turadg Dec 18, 2024

turadg left a comment

0xpatrickdev Dec 18, 2024

turadg Dec 18, 2024

		t.is(makeTestAddress(), 'agoric1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqp7zqht');
		t.is(makeTestAddress(1), 'agoric1qyqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqc09z0g');

	makeAddressFn,
	makeAddressFn = index => `${LOCALCHAIN_DEFAULT_ADDRESS}${index \|\| ''}`

	const cust1 = makeCustomer('Carl', cctp, txPub.publisher, feeConfig, storage);
	const cust1 = makeCustomer('Carl', cctp, txPub.publisher, feeConfig);

chore: advancer validates settlementAccount address #10723

chore: advancer validates settlementAccount address #10723

Conversation

0xpatrickdev 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chore: `advancer` validates `settlementAccount` address #10723

chore: `advancer` validates `settlementAccount` address #10723

0xpatrickdev commented Dec 17, 2024 •

edited

Loading

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

edited

Loading

0xpatrickdev Dec 18, 2024 •

edited

Loading