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

feat: add requestId parameter and relevant validation #62

Merged
merged 2 commits into from
Jun 27, 2024
Merged
Changes from all commits
Commits
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: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
@@ -53,6 +53,7 @@
"reflect-metadata": "^0.2.2",
"sanitize-html": "^2.13.0",
"utility-types": "^3.11.0",
"uuid": "^10.0.0",
"why-is-node-running": "^2.2.2"
},
"devDependencies": {
@@ -69,6 +70,7 @@
"@types/node-cron": "^3.0.11",
"@types/sanitize-html": "^2.11.0",
"@types/sinon": "^17.0.3",
"@types/uuid": "^10.0.0",
"@types/validator": "^13.12.0",
"@types/websocket": "^1.0.10",
"@types/ws": "^8.5.10",
7,714 changes: 3,447 additions & 4,267 deletions pnpm-lock.yaml

Large diffs are not rendered by default.

17 changes: 13 additions & 4 deletions src/controllers/transactions.ts
Original file line number Diff line number Diff line change
@@ -39,7 +39,7 @@ import { getName } from "../krist/names/index.js";
import { areTransactionsEnabled } from "../krist/switches.js";
import { pushTransaction } from "../krist/transactions/create.js";
import { getTransaction, getTransactions, getTransactionsByAddress } from "../krist/transactions/index.js";
import { isValidKristAddress, METANAME_METADATA_RE, NAME_META_RE, validateLimitOffset } from "../utils/index.js";
import { isValidKristAddress, METANAME_METADATA_RE, NAME_META_RE, REQUEST_ID_RE, validateLimitOffset } from "../utils/index.js";
import { checkTxRateLimits } from "../utils/rateLimit.js";

export async function ctrlGetTransactions(
@@ -92,7 +92,8 @@ export async function ctrlMakeTransaction(
privatekey?: string,
recipient?: string,
rawAmount?: string | number,
metadata?: string
metadata?: string,
requestId?: string
): Promise<Transaction> {
if (!await areTransactionsEnabled()) throw new ErrorTransactionsDisabled();

@@ -101,6 +102,9 @@ export async function ctrlMakeTransaction(
if (!recipient) throw new ErrorMissingParameter("to");
if (!rawAmount) throw new ErrorMissingParameter("amount");

if (requestId && !REQUEST_ID_RE.test(requestId))
throw new ErrorInvalidParameter("requestId");

// Check if we're paying to a name
const isName = NAME_META_RE.test(recipient.toLowerCase());
// Handle the potential legacy behavior of manually paying to a name via the
@@ -167,7 +171,8 @@ export async function ctrlMakeTransaction(
amount,
metadata,
undefined,
metaname, dbName.name
metaname, dbName.name,
requestId
);
});
} else {
@@ -179,7 +184,11 @@ export async function ctrlMakeTransaction(
sender.address,
recipient,
amount,
metadata
metadata,
undefined,
undefined,
undefined,
requestId
);
});
}
4 changes: 4 additions & 0 deletions src/database/schemas.ts
Original file line number Diff line number Diff line change
@@ -178,6 +178,10 @@ export class Transaction extends Model<InferAttributes<Transaction>, InferCreati
@Index({ name: "transactions_sent_metaname_sent_name" })
@Attribute(DataTypes.STRING(64))
declare sent_name?: string | null;

@Index({ unique: true })
@Attribute(DataTypes.UUID)
declare request_id?: string | null;
}

// =============================================================================
12 changes: 12 additions & 0 deletions src/errors/transactions.ts
Original file line number Diff line number Diff line change
@@ -38,3 +38,15 @@ export class ErrorTransactionsDisabled extends KristError {
super("Transactions disabled", "transactions_disabled", 423);
}
}

export class ErrorTransactionConflict extends KristError<{
parameter: string;
}> {
constructor(
public parameter: string,
public message = `Transaction conflict for parameter ${parameter}`,
public errorString = "transaction_conflict"
) {
super(message, errorString, 409, { parameter });
}
}
50 changes: 43 additions & 7 deletions src/krist/transactions/create.ts
Original file line number Diff line number Diff line change
@@ -25,7 +25,7 @@ import { Request } from "express";
import PQueue from "p-queue";
import promClient from "prom-client";
import { Address, SqTransaction, Transaction } from "../../database/index.js";
import { ErrorAddressNotFound, ErrorInsufficientFunds } from "../../errors/index.js";
import { ErrorAddressNotFound, ErrorInsufficientFunds, ErrorTransactionConflict } from "../../errors/index.js";
import { criticalLog } from "../../utils/criticalLog.js";
import { getLogDetails } from "../../utils/index.js";
import { TRANSACTION_MAX_CONCURRENCY } from "../../utils/vars.js";
@@ -67,10 +67,21 @@ export async function pushTransaction(
metadata?: string | null,
name?: string | null,
sentMetaname?: string | null,
sentName?: string | null
sentName?: string | null,
requestId?: string | null
): Promise<Transaction> {
return txQueue.add(() => pushTransactionInternal(req, dbTx, senderAddress, recipientAddress, amount, metadata, name,
sentMetaname, sentName), { throwOnTimeout: true });
return txQueue.add(() => pushTransactionInternal(
req,
dbTx,
senderAddress,
recipientAddress,
amount,
metadata,
name,
sentMetaname,
sentName,
requestId
), { throwOnTimeout: true });
}

async function pushTransactionInternal(
@@ -82,7 +93,8 @@ async function pushTransactionInternal(
metadata?: string | null,
name?: string | null,
sentMetaname?: string | null,
sentName?: string | null
sentName?: string | null,
requestId?: string | null
): Promise<Transaction> {
// Fetch the sender from the database. This should also be checked by the caller anyway (name purchase/transfer,
// transaction sending, etc.), but it is important to re-fetch here so that the balance can be checked as part of the
@@ -116,6 +128,27 @@ async function pushTransactionInternal(
throw new ErrorInsufficientFunds();
}

// Check the request ID is either 1. not set, or 2. unused, or 3. idempotent with respect to the transaction details
if (requestId) {
const existingTx = await Transaction.findOne({
where: { request_id: requestId },
transaction: dbTx
});

if (existingTx) {
if (existingTx.from != senderAddress) throw new ErrorTransactionConflict("from");
if (existingTx.to != recipientAddress) throw new ErrorTransactionConflict("to");
if (existingTx.value != amount) throw new ErrorTransactionConflict("amount");
if (existingTx.name != name) throw new ErrorTransactionConflict("name");
if (existingTx.sent_metaname != sentMetaname) throw new ErrorTransactionConflict("sent_metaname");
if (existingTx.sent_name != sentName) throw new ErrorTransactionConflict("sent_name");
if (existingTx.op != metadata) throw new ErrorTransactionConflict("metadata");

// Existing transaction is idempotent, so return it
return existingTx;
}
}

// Find the recipient. If it doesn't exist, it will be created later
recipientAddress = recipientAddress.trim().toLowerCase();
const recipient = await Address.findOne({
@@ -136,7 +169,8 @@ async function pushTransactionInternal(
sender.address,
amount,
name, metadata,
sentMetaname, sentName
sentMetaname, sentName,
requestId
);

if (!recipient) {
@@ -169,7 +203,8 @@ export async function createTransaction(
name?: string | null,
op?: string | null,
sentMetaname?: string | null,
sentName?: string | null
sentName?: string | null,
requestId?: string | null
): Promise<Transaction> {
const { logDetails, userAgent, libraryAgent, origin } = getLogDetails(req);

@@ -187,6 +222,7 @@ export async function createTransaction(
op,
sent_metaname: sentMetaname,
sent_name: sentName,
request_id: requestId,
useragent: userAgent,
library_agent: libraryAgent,
origin
1 change: 1 addition & 0 deletions src/utils/validationKrist.ts
Original file line number Diff line number Diff line change
@@ -27,6 +27,7 @@ export const NAME_FETCH_RE = /^(?:xn--)?[a-z0-9]{1,64}$/i;
export const NAME_A_RECORD_RE = /^[^\s.?#].[^\s]*$/i;
export const NAME_META_RE = /^(?:([a-z0-9-_]{1,32})@)?([a-z0-9]{1,64})\.kst$/i;
export const METANAME_METADATA_RE = /^(?:([a-z0-9-_]{1,32})@)?([a-z0-9]{1,64})\.kst/i;
export const REQUEST_ID_RE = /^[a-z0-9]{8}-[a-z0-9]{4}-[a-z0-9]{4}-[a-z0-9]{4}-[a-z0-9]{12}$/;

export function isValidKristAddress(address: string, v2Only?: boolean): boolean {
return v2Only ? ADDRESS_RE_V2.test(address) : ADDRESS_RE.test(address);
13 changes: 12 additions & 1 deletion src/webserver/routes/transactions.ts
Original file line number Diff line number Diff line change
@@ -256,6 +256,9 @@ export default (): Router => {
* recipient.
* @apiBody {String} [metadata] Optional metadata to include
* in the transaction.
* @apiBody {String} [requestId] Optional request ID to use
* for this transaction. If not provided, a random UUID will
* be generated. Must be a valid UUIDv4 if provided.
*
* @apiUse Transaction
*
@@ -268,6 +271,13 @@ export default (): Router => {
* {
* "ok": false,
* "error": "insufficient_funds"
* }
*
* @apiErrorExample {json} Transaction Conflict
* {
* "ok": false,
* "error": "transaction_conflict",
* "parameter": "amount"
* }
*/
router.post("/transactions", async (req, res) => {
@@ -276,7 +286,8 @@ export default (): Router => {
req.body.privatekey,
req.body.to,
req.body.amount,
req.body.metadata
req.body.metadata,
req.body.requestId
);

res.json({
14 changes: 13 additions & 1 deletion src/websockets/routes/transactions.ts
Original file line number Diff line number Diff line change
@@ -39,6 +39,9 @@ import { WebSocketEventHandler } from "../types.js";
* recipient.
* @apiBody {String} [metadata] Optional metadata to
* include in the transaction.
* @apiBody {String} [requestId] Optional request ID to use
* for this transaction. If not provided, a random UUID will
* be generated. Must be a valid UUIDv4 if provided.
*
* @apiUse Transaction
*
@@ -52,12 +55,20 @@ import { WebSocketEventHandler } from "../types.js";
* "ok": false,
* "error": "insufficient_funds"
* }
*
* @apiErrorExample {json} Transaction Conflict
* {
* "ok": false,
* "error": "transaction_conflict",
* "parameter": "amount"
* }
*/
export const wsMakeTransaction: WebSocketEventHandler<{
privatekey?: string;
to?: string;
amount?: string | number;
metadata?: string;
requestId?: string;
}> = async (ws, msg) => {
if (ws.isGuest && !msg.privatekey)
throw new ErrorMissingParameter("privatekey");
@@ -67,7 +78,8 @@ export const wsMakeTransaction: WebSocketEventHandler<{
msg.privatekey || ws.privatekey,
msg.to,
msg.amount,
msg.metadata
msg.metadata,
msg.requestId
);

return {
77 changes: 77 additions & 0 deletions test/routes/transactions.test.ts
Original file line number Diff line number Diff line change
@@ -26,6 +26,7 @@ import { api } from "../api.js";
import { Name, Address, Transaction, db } from "../../src/database/index.js";
import { redis, rKey } from "../../src/database/redis.js";
import { pick } from "lodash-es";
import { v4 as uuidv4 } from "uuid";

const expectTransactionExist = (
id: number,
@@ -206,6 +207,15 @@ describe("v2 routes: transactions", () => {
expect(res).to.be.json;
expect(res.body).to.deep.include({ ok: false, error: "name_not_found" });
});

it("should error when using an invalid request ID", async () => {
const res = await api()
.post("/transactions")
.send({ amount: 1, to: "k7oax47quv", privatekey: "a", requestId: "invalid" });

expect(res).to.be.json;
expect(res.body).to.deep.include({ ok: false, error: "invalid_parameter", parameter: "requestId" });
});
});

describe("POST /transactions", () => {
@@ -367,6 +377,73 @@ describe("v2 routes: transactions", () => {
expect(res.body.transaction.metadata).to.equal("test.kst;notfound.kst");
expect(res.body.transaction.sent_name).to.equal("test");
});

it("should transact idempotently when using a request ID", async () => {
// Get previous balance
let from = await Address.findOne({ where: { address: "kvhccnbm95" }});
const priorBalance = from!.balance;
const expectedBalance = priorBalance - 1;

// Submit the transaction
const requestId = uuidv4();
const res1 = await api()
.post("/transactions")
.send({ amount: 1, to: "kkkkkkkkkk", privatekey: "z", requestId });

expect(res1).to.be.json;
expect(res1.body).to.deep.include({ ok: true });
expect(res1.body.transaction).to.be.an("object");
const txId = res1.body.transaction.id;

// Get new balance
from = await Address.findOne({ where: { address: "kvhccnbm95" }});
expect(from!.balance).to.equal(expectedBalance);

// Get transaction
const tx = await Transaction.findOne({ where: { request_id: requestId } });
expect(tx).to.exist;
expect(tx!.id).to.equal(txId);
expect(tx!.from).to.equal("kvhccnbm95");
expect(tx!.to).to.equal("kkkkkkkkkk");
expect(tx!.value).to.equal(1);
expect(tx!.request_id).to.equal(requestId);

// Re-submit the transaction
const res2 = await api()
.post("/transactions")
.send({ amount: 1, to: "kkkkkkkkkk", privatekey: "z", requestId });

expect(res2).to.be.json;
expect(res2.body).to.deep.include({ ok: true });

// Can't compare time since we lose precision after saving to the database
delete res1.body.transaction.time;
delete res2.body.transaction.time;
expect(res2.body).to.deep.equal(res1.body);

// Make sure the balance didn't change
from = await Address.findOne({ where: { address: "kvhccnbm95" }});
expect(from!.balance).to.equal(expectedBalance);
});

it("should fail to transact when using a request ID if different request details are used", async () => {
// Submit the transaction
const requestId = uuidv4();
const res1 = await api()
.post("/transactions")
.send({ amount: 1, to: "kkkkkkkkkk", privatekey: "z", requestId });

expect(res1).to.be.json;
expect(res1.body).to.deep.include({ ok: true });

// Re-submit the transaction
const res2 = await api()
.post("/transactions")
.send({ amount: 2, to: "kkkkkkkkkk", privatekey: "z", requestId });

expect(res2).to.be.json;
expect(res2.body).to.deep.include({ ok: false, error: "transaction_conflict" });
});
});
});

1 change: 1 addition & 0 deletions test/seed.ts
Original file line number Diff line number Diff line change
@@ -54,6 +54,7 @@ export async function seed(): Promise<void> {
{ address: "k7oax47quv", balance: 0, totalin: 0, totalout: 0, firstseen: new Date(), privatekey: "1f71334443b70c5c384894bc6308e9fcfb5b3103abb82eba6cd26d7767b5740c" },
{ address: "kwsgj3x184", balance: 0, totalin: 0, totalout: 0, firstseen: new Date(), privatekey: "75185375f6e1e0eecbbe875355de2e38b7e548efbc80985479f5870967dcd2df", alert: "Test alert", locked: true },
{ address: "k0duvsr4qn", balance: 25000, totalin: 0, totalout: 0, firstseen: new Date(), privatekey: "4827fb69dbc85b39204595dc870029d2a390a67b5275bd4588ae6567c01397d5" },
{ address: "kvhccnbm95", balance: 100, totalin: 0, totalout: 0, firstseen: new Date(), privatekey: "c2b6cff7afc28c764ae239da2e785c1002f137dd9828416321f6c9a96b26f76e" },
])
]);