Skip to content
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

misc fastUsdc vstorage cleanup #10661

Merged
merged 10 commits into from
Dec 10, 2024
1 change: 0 additions & 1 deletion packages/boot/test/bootstrapTests/vaults-upgrade.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
/* eslint-disable @jessie.js/safe-await-separator */
/**
* @file Bootstrap test integration vaults with smart-wallet. The tests in this
* file are NOT independent; a single `test.before()` handler creates shared
Expand Down
1 change: 0 additions & 1 deletion packages/boot/test/bootstrapTests/vtransfer.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
/* eslint-disable @jessie.js/safe-await-separator -- confused by casting 'as' */
import { test as anyTest } from '@agoric/zoe/tools/prepare-test-env-ava.js';

import type { TestFn } from 'ava';
Expand Down
1 change: 0 additions & 1 deletion packages/boot/test/upgrading/upgrade-vats.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
/* eslint-disable @jessie.js/safe-await-separator -- test */
import { test } from '@agoric/swingset-vat/tools/prepare-test-env-ava.js';

import { BridgeId, deepCopyJsonable } from '@agoric/internal';
Expand Down
88 changes: 53 additions & 35 deletions packages/fast-usdc/src/exos/status-manager.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,21 +13,29 @@ import { PendingTxStatus, TxStatus } from '../constants.js';
/**
* @import {MapStore, SetStore} from '@agoric/store';
* @import {Zone} from '@agoric/zone';
* @import {CctpTxEvidence, NobleAddress, SeenTxKey, PendingTxKey, PendingTx, EvmHash, LogFn} from '../types.js';
* @import {CctpTxEvidence, NobleAddress, PendingTx, EvmHash, LogFn} from '../types.js';
*/

/**
* @typedef {`pendingTx:${bigint}:${NobleAddress}`} PendingTxKey
* The string template is for developer visibility but not meant to ever be parsed.
*
* @typedef {`seenTx:${string}:${EvmHash}`} SeenTxKey
* The string template is for developer visibility but not meant to ever be parsed.
*/

/**
* Create the key for the pendingTxs MapStore.
*
* The key is a composite of `txHash` and `chainId` and not meant to be
* parsable.
* The key is a composite but not meant to be parsable.
*
* @param {NobleAddress} addr
* @param {bigint} amount
* @returns {PendingTxKey}
*/
const makePendingTxKey = (addr, amount) =>
`pendingTx:${JSON.stringify([addr, String(amount)])}`;
// amount can't contain colon
`pendingTx:${amount}:${addr}`;

/**
* Get the key for the pendingTxs MapStore.
Expand All @@ -43,15 +51,15 @@ const pendingTxKeyOf = evidence => {
/**
* Get the key for the seenTxs SetStore.
*
* The key is a composite of `NobleAddress` and transaction `amount` and not
* meant to be parsable.
* The key is a composite but not meant to be parsable.
*
* @param {CctpTxEvidence} evidence
* @returns {SeenTxKey}
*/
const seenTxKeyOf = evidence => {
const { txHash, chainId } = evidence;
return `seenTx:${JSON.stringify([txHash, chainId])}`;
// chainId can't contain colon
return `seenTx:${chainId}:${txHash}`;
};

/**
Expand All @@ -68,12 +76,12 @@ const seenTxKeyOf = evidence => {
* XXX consider separate facets for `Advancing` and `Settling` capabilities.
*
* @param {Zone} zone
* @param {() => Promise<StorageNode>} makeStatusNode
* @param {ERef<StorageNode>} transactionsNode
* @param {StatusManagerPowers} caps
*/
export const prepareStatusManager = (
zone,
makeStatusNode,
transactionsNode,
{
log = makeTracer('Advancer', true),
} = /** @type {StatusManagerPowers} */ ({}),
Expand All @@ -93,9 +101,8 @@ export const prepareStatusManager = (
* @param {CctpTxEvidence['txHash']} hash
* @param {TxStatus} status
*/
const recordStatus = (hash, status) => {
const statusNodeP = makeStatusNode();
const txnNodeP = E(statusNodeP).makeChildNode(hash);
const publishStatus = (hash, status) => {
const txnNodeP = E(transactionsNode).makeChildNode(hash);
// Don't await, just writing to vstorage.
void E(txnNodeP).setValue(status);
};
Expand All @@ -109,7 +116,7 @@ export const prepareStatusManager = (
* @param {CctpTxEvidence} evidence
* @param {PendingTxStatus} status
*/
const recordPendingTx = (evidence, status) => {
const initPendingTx = (evidence, status) => {
const seenKey = seenTxKeyOf(evidence);
if (seenTxs.has(seenKey)) {
throw makeError(`Transaction already seen: ${q(seenKey)}`);
Expand All @@ -121,9 +128,31 @@ export const prepareStatusManager = (
pendingTxKeyOf(evidence),
harden({ ...evidence, status }),
);
recordStatus(evidence.txHash, status);
publishStatus(evidence.txHash, status);
};

/**
* Update the pending transaction status.
*
* @param {{sender: NobleAddress, amount: bigint}} keyParts
* @param {PendingTxStatus} status
*/
function setPendingTxStatus({ sender, amount }, status) {
const key = makePendingTxKey(sender, amount);
pendingTxs.has(key) || Fail`no advancing tx with ${{ sender, amount }}`;
const pending = pendingTxs.get(key);
const ix = pending.findIndex(tx => tx.status === PendingTxStatus.Advancing);
ix >= 0 || Fail`no advancing tx with ${{ sender, amount }}`;
const [prefix, tx, suffix] = [
pending.slice(0, ix),
pending[ix],
pending.slice(ix + 1),
];
const txpost = { ...tx, status };
pendingTxs.set(key, harden([...prefix, txpost, ...suffix]));
publishStatus(tx.txHash, status);
}

return zone.exo(
'Fast USDC Status Manager',
M.interface('StatusManagerI', {
Expand Down Expand Up @@ -156,10 +185,13 @@ export const prepareStatusManager = (
{
/**
* Add a new transaction with ADVANCING status
*
* NB: this acts like observe() but skips recording the OBSERVED state
*
* @param {CctpTxEvidence} evidence
*/
advance(evidence) {
recordPendingTx(evidence, PendingTxStatus.Advancing);
initPendingTx(evidence, PendingTxStatus.Advancing);
},

/**
Expand All @@ -171,32 +203,18 @@ export const prepareStatusManager = (
* @throws {Error} if nothing to advance
*/
advanceOutcome(sender, amount, success) {
const key = makePendingTxKey(sender, amount);
pendingTxs.has(key) || Fail`no advancing tx with ${{ sender, amount }}`;
const pending = pendingTxs.get(key);
const ix = pending.findIndex(
tx => tx.status === PendingTxStatus.Advancing,
setPendingTxStatus(
{ sender, amount },
success ? PendingTxStatus.Advanced : PendingTxStatus.AdvanceFailed,
);
ix >= 0 || Fail`no advancing tx with ${{ sender, amount }}`;
const [prefix, tx, suffix] = [
pending.slice(0, ix),
pending[ix],
pending.slice(ix + 1),
];
const status = success
? PendingTxStatus.Advanced
: PendingTxStatus.AdvanceFailed;
const txpost = { ...tx, status };
pendingTxs.set(key, harden([...prefix, txpost, ...suffix]));
recordStatus(tx.txHash, status);
},

/**
* Add a new transaction with OBSERVED status
* @param {CctpTxEvidence} evidence
*/
observe(evidence) {
recordPendingTx(evidence, PendingTxStatus.Observed);
initPendingTx(evidence, PendingTxStatus.Observed);
},

/**
Expand Down Expand Up @@ -247,7 +265,7 @@ export const prepareStatusManager = (
* @param {EvmHash} txHash
*/
disbursed(txHash) {
recordStatus(txHash, TxStatus.Disbursed);
publishStatus(txHash, TxStatus.Disbursed);
},

/**
Expand All @@ -259,7 +277,7 @@ export const prepareStatusManager = (
*/
forwarded(txHash, address, amount) {
if (txHash) {
recordStatus(txHash, TxStatus.Forwarded);
publishStatus(txHash, TxStatus.Forwarded);
} else {
// TODO store (early) `Minted` transactions to check against incoming evidence
log(
Expand Down
4 changes: 2 additions & 2 deletions packages/fast-usdc/src/fast-usdc.contract.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,8 @@ export const contract = async (zcf, privateArgs, zone, tools) => {
marshaller,
);

const makeStatusNode = () => E(storageNode).makeChildNode(STATUS_NODE);
const statusManager = prepareStatusManager(zone, makeStatusNode);
const statusNode = E(storageNode).makeChildNode(STATUS_NODE);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused about line 178: "Define all kinds above this line. Keep remote calls below."

Seems like this would go against that because it's a remote call. Does it matter? The makeStatusNode was already kind of a gray area in that it wasn't called immediately.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That comment really meant "keep remote call awaits below". We'll need to improve the comment and patterns for contract developers about that, but it's out of scope for this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thanks for clarifying.

const statusManager = prepareStatusManager(zone, statusNode);

const { USDC } = terms.brands;
const { withdrawToSeat } = tools.zoeTools;
Expand Down
6 changes: 0 additions & 6 deletions packages/fast-usdc/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,6 @@ export interface PendingTx extends CctpTxEvidence {
status: PendingTxStatus;
}

/** internal key for `StatusManager` exo */
export type PendingTxKey = `pendingTx:${string}`;

/** internal key for `StatusManager` exo */
export type SeenTxKey = `seenTx:${string}`;

export type FeeConfig = {
flat: Amount<'nat'>;
variableRate: Ratio;
Expand Down
2 changes: 1 addition & 1 deletion packages/fast-usdc/src/util/agoric.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ export const queryFastUSDCLocalChainAccount = async (
out = console,
) => {
const agoricAddr = await vstorage.readLatest(
'published.fastUSDC.settlementAccount',
'published.fastUsdc.settlementAccount',
);
out.log(`Got Fast USDC Local Chain Account ${agoricAddr}`);
return agoricAddr;
Expand Down
4 changes: 2 additions & 2 deletions packages/fast-usdc/test/cli/transfer.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ test('Transfer registers the noble forwarding account if it does not exist', asy
const out = mockOut();
const file = mockFile(path, JSON.stringify(config));
const agoricSettlementAccount = 'agoric123456';
const settlementAccountVstoragePath = 'published.fastUSDC.settlementAccount';
const settlementAccountVstoragePath = 'published.fastUsdc.settlementAccount';
const vstorageMock = makeVstorageMock({
[settlementAccountVstoragePath]: agoricSettlementAccount,
});
Expand Down Expand Up @@ -115,7 +115,7 @@ test('Transfer signs and broadcasts the depositForBurn message on Ethereum', asy
const out = mockOut();
const file = mockFile(path, JSON.stringify(config));
const agoricSettlementAccount = 'agoric123456';
const settlementAccountVstoragePath = 'published.fastUSDC.settlementAccount';
const settlementAccountVstoragePath = 'published.fastUsdc.settlementAccount';
const vstorageMock = makeVstorageMock({
[settlementAccountVstoragePath]: agoricSettlementAccount,
});
Expand Down
52 changes: 16 additions & 36 deletions packages/fast-usdc/test/exos/advancer.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ const createTestExtensions = (t, common: CommonSetup) => {

const statusManager = prepareStatusManager(
rootZone.subZone('status-manager'),
async () => storageNode.makeChildNode('status'),
storageNode.makeChildNode('status'),
);

const mockAccounts = prepareMockOrchAccounts(rootZone.subZone('accounts'), {
Expand Down Expand Up @@ -162,6 +162,7 @@ test('updates status to ADVANCING in happy path', async t => {
mocks: { mockPoolAccount, resolveLocalTransferV },
},
brands: { usdc },
bootstrap: { storage },
} = t.context;

const mockEvidence = MockCctpTxEvidences.AGORIC_PLUS_OSMO();
Expand All @@ -174,14 +175,9 @@ test('updates status to ADVANCING in happy path', async t => {
// wait for handleTransactionEvent to do work
await eventLoopIteration();

const entries = statusManager.lookupPending(
mockEvidence.tx.forwardingAddress,
mockEvidence.tx.amount,
);

t.deepEqual(
entries,
[{ ...mockEvidence, status: PendingTxStatus.Advancing }],
storage.data.get(`mockChainStorageRoot.status.${mockEvidence.txHash}`),
PendingTxStatus.Advancing,
'ADVANCED status in happy path',
);

Expand Down Expand Up @@ -210,6 +206,7 @@ test('updates status to ADVANCING in happy path', async t => {

test('updates status to OBSERVED on insufficient pool funds', async t => {
const {
bootstrap: { storage },
extensions: {
services: { makeAdvancer, statusManager },
helpers: { inspectLogs },
Expand All @@ -229,14 +226,9 @@ test('updates status to OBSERVED on insufficient pool funds', async t => {
void advancer.handleTransactionEvent(mockEvidence);
await eventLoopIteration();

const entries = statusManager.lookupPending(
mockEvidence.tx.forwardingAddress,
mockEvidence.tx.amount,
);

t.deepEqual(
entries,
[{ ...mockEvidence, status: PendingTxStatus.Observed }],
storage.data.get(`mockChainStorageRoot.status.${mockEvidence.txHash}`),
PendingTxStatus.Observed,
'OBSERVED status on insufficient pool funds',
);

Expand All @@ -248,6 +240,7 @@ test('updates status to OBSERVED on insufficient pool funds', async t => {

test('updates status to OBSERVED if makeChainAddress fails', async t => {
const {
bootstrap: { storage },
extensions: {
services: { advancer, statusManager },
helpers: { inspectLogs },
Expand All @@ -257,14 +250,9 @@ test('updates status to OBSERVED if makeChainAddress fails', async t => {
const mockEvidence = MockCctpTxEvidences.AGORIC_UNKNOWN_EUD();
await advancer.handleTransactionEvent(mockEvidence);

const entries = statusManager.lookupPending(
mockEvidence.tx.forwardingAddress,
mockEvidence.tx.amount,
);

t.deepEqual(
entries,
[{ ...mockEvidence, status: PendingTxStatus.Observed }],
storage.data.get(`mockChainStorageRoot.status.${mockEvidence.txHash}`),
PendingTxStatus.Observed,
'OBSERVED status on makeChainAddress failure',
);

Expand All @@ -276,6 +264,7 @@ test('updates status to OBSERVED if makeChainAddress fails', async t => {

test('calls notifyAdvancingResult (AdvancedFailed) on failed transfer', async t => {
const {
bootstrap: { storage },
extensions: {
services: { advancer, feeTools, statusManager },
helpers: { inspectLogs, inspectNotifyCalls },
Expand All @@ -291,14 +280,9 @@ test('calls notifyAdvancingResult (AdvancedFailed) on failed transfer', async t
resolveLocalTransferV();
await eventLoopIteration();

const entries = statusManager.lookupPending(
mockEvidence.tx.forwardingAddress,
mockEvidence.tx.amount,
);

t.deepEqual(
entries,
[{ ...mockEvidence, status: PendingTxStatus.Advancing }],
storage.data.get(`mockChainStorageRoot.status.${mockEvidence.txHash}`),
PendingTxStatus.Advancing,
'tx is Advancing',
);

Expand Down Expand Up @@ -333,6 +317,7 @@ test('calls notifyAdvancingResult (AdvancedFailed) on failed transfer', async t

test('updates status to OBSERVED if pre-condition checks fail', async t => {
const {
bootstrap: { storage },
extensions: {
services: { advancer, statusManager },
helpers: { inspectLogs },
Expand All @@ -343,14 +328,9 @@ test('updates status to OBSERVED if pre-condition checks fail', async t => {

await advancer.handleTransactionEvent(mockEvidence);

const entries = statusManager.lookupPending(
mockEvidence.tx.forwardingAddress,
mockEvidence.tx.amount,
);

t.deepEqual(
entries,
[{ ...mockEvidence, status: PendingTxStatus.Observed }],
storage.data.get(`mockChainStorageRoot.status.${mockEvidence.txHash}`),
PendingTxStatus.Observed,
'tx is recorded as OBSERVED',
);

Expand Down
Loading
Loading