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.

Sign up for GitHub

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

record transaction fees #10715

Merged
merged 6 commits into from
Dec 17, 2024
Merged

record transaction fees #10715

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
02c111b
feat: storage-test-utils report missing data
turadg Dec 17, 2024
12627ee
test: DRY setup of status-manager test
turadg Dec 17, 2024
8c5bf85
test: quieter advancer test
turadg Dec 17, 2024
28cc2f6
refactor: inline publishStatus
turadg Dec 17, 2024
e3f49be
refactor: full marshaller
turadg Dec 17, 2024
8846972
feat: record fee split in transaction
turadg Dec 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/fast-usdc/src/exos/settler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ export const prepareSettler = (
repayer.repay(settlingSeat, split);

// update status manager, marking tx `SETTLED`
statusManager.disbursed(txHash);
statusManager.disbursed(txHash, split);
},
/**
* @param {EvmHash} txHash
Expand Down
64 changes: 34 additions & 30 deletions packages/fast-usdc/src/exos/status-manager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
import { M } from '@endo/patterns';
import { Fail, makeError, q } from '@endo/errors';
import { makeTracer } from '@agoric/internal';
import { appendToStoredArray } from '@agoric/store/src/stores/store-utils.js';
import { AmountKeywordRecordShape } from '@agoric/zoe/src/typeGuards.js';
import { Fail, makeError, q } from '@endo/errors';
import { E } from '@endo/eventual-send';
import { makeTracer, pureDataMarshaller } from '@agoric/internal';
import { M } from '@endo/patterns';
import { PendingTxStatus, TerminalTxStatus, TxStatus } from '../constants.js';
import {
CctpTxEvidenceShape,
EvmHashShape,
PendingTxShape,
} from '../type-guards.js';
import { PendingTxStatus, TerminalTxStatus, TxStatus } from '../constants.js';

/**
* @import {MapStore, SetStore} from '@agoric/store';
Expand Down Expand Up @@ -48,6 +49,7 @@ const pendingTxKeyOf = evidence => {
/**
* @typedef {{
* log?: LogFn;
* marshaller: ERef<Marshaller>;
* }} StatusManagerPowers
*/

Expand All @@ -66,6 +68,7 @@ export const prepareStatusManager = (
zone,
txnsNode,
{
marshaller,
log = makeTracer('Advancer', true),
} = /** @type {StatusManagerPowers} */ ({}),
) => {
Expand Down Expand Up @@ -102,14 +105,25 @@ export const prepareStatusManager = (
/**
* @param {EvmHash} txId
* @param {TransactionRecord} record
* @returns {Promise<void>}
*/
const publishTxnRecord = (txId, record) => {
const publishTxnRecord = async (txId, record) => {
const txNode = E(txnsNode).makeChildNode(txId, {
sequence: true, // avoid overwriting other output in the block
});
void E(txNode).setValue(
JSON.stringify(pureDataMarshaller.toCapData(record)),
);

// XXX awkward for publish* to update a store, but it's temporary
if (record.status && TerminalTxStatus[record.status]) {
// UNTIL https://github.com/Agoric/agoric-sdk/issues/7405
// Queue it for deletion later because if we deleted it now the earlier
// writes in this block would be wiped. For now we keep track of what to
// delete when we know it'll be another block.
storedCompletedTxs.add(txId);
}

const capData = await E(marshaller).toCapData(record);

Copy link
Member

Choose a reason for hiding this comment

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

side note: This adds a round-trip to the board vat; I'm curious whether it has a noticeable / measurable performance impact. Here's hoping I/we measure that in...

Copy link
Member Author

Choose a reason for hiding this comment

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

💯

await E(txNode).setValue(JSON.stringify(capData));
};
Copy link
Member

Choose a reason for hiding this comment

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

why await the setValue?

I'm fuzzy on our void vs await conventions.

Copy link
Member Author

Choose a reason for hiding this comment

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

the function has to be async to get capData before stringifying it.

this final await is just to provide the option to the caller to await that part. All the consumers use void now but voiding here wouldn't change the need for that.


/**
Expand All @@ -124,22 +138,6 @@ export const prepareStatusManager = (
);
};

/**
* @param {CctpTxEvidence['txHash']} hash
* @param {TxStatus} status
*/
const publishStatus = (hash, status) => {
// Don't await, just writing to vstorage.
void publishTxnRecord(hash, harden({ status }));
if (TerminalTxStatus[status]) {
// UNTIL https://github.com/Agoric/agoric-sdk/issues/7405
// Queue it for deletion later because if we deleted it now the earlier
// writes in this block would be wiped. For now we keep track of what to
// delete when we know it'll be another block.
storedCompletedTxs.add(hash);
}
};

/**
* Ensures that `txHash+chainId` has not been processed
* and adds entry to `seenTxs` set.
Expand All @@ -164,7 +162,7 @@ export const prepareStatusManager = (
publishEvidence(txHash, evidence);
if (status !== PendingTxStatus.Observed) {
// publishEvidence publishes Observed
publishStatus(txHash, status);
void publishTxnRecord(txHash, harden({ status }));
}
Copy link
Member

Choose a reason for hiding this comment

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

does void mean "I'm certain this never rejects" or "if it rejects, there's nothing useful I can do, so I might as well let standard unhandled rejection logging do its thing"?

Copy link
Member Author

Choose a reason for hiding this comment

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

the latter

};

Expand All @@ -187,7 +185,7 @@ export const prepareStatusManager = (
];
const txpost = { ...tx, status };
pendingTxs.set(key, harden([...prefix, txpost, ...suffix]));
publishStatus(tx.txHash, status);
void publishTxnRecord(tx.txHash, harden({ status }));
}

return zone.exo(
Expand All @@ -212,7 +210,9 @@ export const prepareStatusManager = (
M.undefined(),
),
),
disbursed: M.call(EvmHashShape).returns(M.undefined()),
disbursed: M.call(EvmHashShape, AmountKeywordRecordShape).returns(
M.undefined(),
),
forwarded: M.call(M.opt(EvmHashShape), M.string(), M.nat()).returns(
M.undefined(),
),
Expand Down Expand Up @@ -313,9 +313,13 @@ export const prepareStatusManager = (
* Mark a transaction as `DISBURSED`
*
* @param {EvmHash} txHash
* @param {import('./liquidity-pool.js').RepayAmountKWR} split
*/
disbursed(txHash) {
publishStatus(txHash, TxStatus.Disbursed);
disbursed(txHash, split) {
void publishTxnRecord(
txHash,
harden({ split, status: TxStatus.Disbursed }),
);
},

/**
Expand All @@ -327,7 +331,7 @@ export const prepareStatusManager = (
*/
forwarded(txHash, nfa, amount) {
if (txHash) {
publishStatus(txHash, TxStatus.Forwarded);
void publishTxnRecord(txHash, harden({ status: TxStatus.Forwarded }));
} else {
// TODO store (early) `Minted` transactions to check against incoming evidence
log(
Expand Down
1 change: 1 addition & 0 deletions packages/fast-usdc/src/fast-usdc.contract.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ export const contract = async (zcf, privateArgs, zone, tools) => {
const statusManager = prepareStatusManager(
zone,
E(storageNode).makeChildNode(TXNS_NODE),
{ marshaller },
);

const { USDC } = terms.brands;
Expand Down
2 changes: 2 additions & 0 deletions packages/fast-usdc/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import type { Amount } from '@agoric/ertp';
import type { CopyRecord, Passable } from '@endo/pass-style';
import type { PendingTxStatus, TxStatus } from './constants.js';
import type { FastUsdcTerms } from './fast-usdc.contract.js';
import type { RepayAmountKWR } from './exos/liquidity-pool.js';

export type EvmHash = `0x${string}`;
export type EvmAddress = `0x${string & { length: 40 }}`;
Expand Down Expand Up @@ -40,6 +41,7 @@ export interface CctpTxEvidence {
*/
export interface TransactionRecord extends CopyRecord {
Copy link
Member

Choose a reason for hiding this comment

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

This type could be more precise. It could be a union that requires evidence in OBSERVED and split in DISBURSED.

Copy link
Member Author

Choose a reason for hiding this comment

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

could be. I went this way to maintain the option to include these any time.

evidence?: CctpTxEvidence;
split?: RepayAmountKWR;
status: TxStatus;
}

Expand Down
20 changes: 13 additions & 7 deletions packages/fast-usdc/test/exos/advancer.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { type ZoeTools } from '@agoric/orchestration/src/utils/zoe-tools.js';
import { q } from '@endo/errors';
import { Far } from '@endo/pass-style';
import type { TestFn } from 'ava';
import { makeTracer } from '@agoric/internal';
import { PendingTxStatus } from '../../src/constants.js';
import { prepareAdvancer } from '../../src/exos/advancer.js';
import type { SettlerKit } from '../../src/exos/settler.js';
Expand All @@ -26,6 +27,8 @@ import {
} from '../mocks.js';
import { commonSetup } from '../supports.js';

const trace = makeTracer('AdvancerTest', false);

const LOCAL_DENOM = `ibc/${denomHash({
denom: 'uusdc',
channelId:
Expand All @@ -50,6 +53,7 @@ const createTestExtensions = (t, common: CommonSetup) => {
const statusManager = prepareStatusManager(
rootZone.subZone('status-manager'),
storageNode.makeChildNode('txns'),
{ marshaller: common.commonPrivateArgs.marshaller },
);

const mockAccounts = prepareMockOrchAccounts(rootZone.subZone('accounts'), {
Expand All @@ -74,7 +78,7 @@ const createTestExtensions = (t, common: CommonSetup) => {
};
const mockZoeTools = Far('MockZoeTools', {
localTransfer(...args: Parameters<ZoeTools['localTransfer']>) {
console.log('ZoeTools.localTransfer called with', args);
trace('ZoeTools.localTransfer called with', args);
return localTransferVK.vow;
},
});
Expand All @@ -99,7 +103,7 @@ const createTestExtensions = (t, common: CommonSetup) => {
const notifyAdvancingResultCalls: NotifyArgs[] = [];
const mockNotifyF = Far('Settler Notify Facet', {
notifyAdvancingResult: (...args: NotifyArgs) => {
console.log('Settler.notifyAdvancingResult called with', args);
trace('Settler.notifyAdvancingResult called with', args);
notifyAdvancingResultCalls.push(args);
},
});
Expand Down Expand Up @@ -167,7 +171,7 @@ test.beforeEach(async t => {
test('updates status to ADVANCING in happy path', async t => {
const {
extensions: {
services: { advancer, feeTools, statusManager },
services: { advancer, feeTools },
helpers: { inspectLogs, inspectNotifyCalls },
mocks: { mockPoolAccount, resolveLocalTransferV },
},
Expand Down Expand Up @@ -235,7 +239,7 @@ test('updates status to OBSERVED on insufficient pool funds', async t => {
brands: { usdc },
bootstrap: { storage },
extensions: {
services: { makeAdvancer, statusManager },
services: { makeAdvancer },
helpers: { inspectLogs },
mocks: { mockPoolAccount, mockNotifyF },
},
Expand Down Expand Up @@ -283,13 +287,14 @@ test('updates status to OBSERVED if makeChainAddress fails', async t => {
const {
bootstrap: { storage },
extensions: {
services: { advancer, statusManager },
services: { advancer },
helpers: { inspectLogs },
},
} = t.context;

const evidence = MockCctpTxEvidences.AGORIC_UNKNOWN_EUD();
await advancer.handleTransactionEvent(evidence);
await eventLoopIteration();

t.deepEqual(
storage.getDeserialized(`fun.txns.${evidence.txHash}`),
Expand All @@ -310,7 +315,7 @@ test('calls notifyAdvancingResult (AdvancedFailed) on failed transfer', async t
const {
bootstrap: { storage },
extensions: {
services: { advancer, feeTools, statusManager },
services: { advancer, feeTools },
helpers: { inspectLogs, inspectNotifyCalls },
mocks: { mockPoolAccount, resolveLocalTransferV },
},
Expand Down Expand Up @@ -363,14 +368,15 @@ test('updates status to OBSERVED if pre-condition checks fail', async t => {
const {
bootstrap: { storage },
extensions: {
services: { advancer, statusManager },
services: { advancer },
helpers: { inspectLogs },
},
} = t.context;

const evidence = MockCctpTxEvidences.AGORIC_NO_PARAMS();

await advancer.handleTransactionEvent(evidence);
await eventLoopIteration();

t.deepEqual(
storage.getDeserialized(`fun.txns.${evidence.txHash}`),
Expand Down
5 changes: 3 additions & 2 deletions packages/fast-usdc/test/exos/settler.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import type { TestFn } from 'ava';
import { eventLoopIteration } from '@agoric/internal/src/testing-utils.js';
import fetchedChainInfo from '@agoric/orchestration/src/fetched-chain-info.js';
import type { Zone } from '@agoric/zone';
import { defaultMarshaller } from '@agoric/internal/src/storage-test-utils.js';
import { PendingTxStatus } from '../../src/constants.js';
import { prepareSettler } from '../../src/exos/settler.js';
import { prepareStatusManager } from '../../src/exos/status-manager.js';
Expand Down Expand Up @@ -48,7 +49,7 @@ const makeTestContext = async t => {
const statusManager = prepareStatusManager(
zone.subZone('status-manager'),
common.commonPrivateArgs.storageNode.makeChildNode('txns'),
{ log },
{ marshaller: defaultMarshaller, log },
);
const { zcf, callLog } = mockZcf(zone.subZone('Mock ZCF'));

Expand Down Expand Up @@ -239,7 +240,7 @@ test('happy path: disburse to LPs; StatusManager removes tx', async t => {
{ evidence: cctpTxEvidence, status: 'OBSERVED' },
{ status: 'ADVANCING' },
{ status: 'ADVANCED' },
{ status: 'DISBURSED' },
{ split: expectedSplit, status: 'DISBURSED' },
]);

// Check deletion of DISBURSED transactions
Expand Down
Loading
Loading