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

(Multi-)Cashlink improvements #489

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Prev Previous commit
Next Next commit
Cashlink: use fee for parallel cashlink claims
To avoid reaching the free transaction limit per block, use paid
transactions for parallel claims of multi-claimable cashlinks.

Currently trying to detect how many claiming transactions are
pending in parallel, and only applying a fee if they exceed a
certain threshold. However, not sure yet how effective that is
because the balance update is likely finished before we got the
pending transactions. This means detectState might already set
the state to UNCLAIMED and the ui might make the cashlink
claimable before we can check the pending transactions.
If the current check does not proof effective, we might need to
make all multi-cashlink claims paid transactions, e.g. by
detecting multi-cashlinks as cashlink.balance > cashlink.value.
danimoh committed May 30, 2022

Verified

This commit was signed with the committer’s verified signature.
commit 559d523880299209c5c9e27fcddc8c8f18dee3a2
65 changes: 50 additions & 15 deletions src/lib/Cashlink.ts
Original file line number Diff line number Diff line change
@@ -37,12 +37,33 @@ class Cashlink {
set fee(fee: number) {
if (this.state === CashlinkState.CLAIMED) {
console.warn('Setting a fee will typically have no effect anymore as Cashlink is already claimed');
} else if (fee && fee < Cashlink.MIN_PAID_TRANSACTION_FEE) {
console.warn(`Fee of ${Nimiq.Policy.lunasToCoins(fee)} is too low to count as paid transaction`);
} else if (fee < this.suggestedFee) {
console.warn(`Fee of ${Nimiq.Policy.lunasToCoins(fee)} is smaller than suggested fee of `
+ Nimiq.Policy.lunasToCoins(this.fee));
}
this._fee = fee;
}

get fee() {
return this._fee || 0;
return this._fee !== null ? this._fee : this.suggestedFee;
}

get suggestedFee() {
if (
// Cashlink balance was apparently specifically setup such that every claiming transaction can be a paid one
(this.balance && this.balance % (this.value + Cashlink.MIN_PAID_TRANSACTION_FEE) === 0)
// Many parallel claims are happening right now. Because free transactions per sender are limited to 10 per
// block (Mempool.FREE_TRANSACTIONS_PER_SENDER_MAX) we better send further transactions with a fee. Using
// 5 as cutoff instead of 10 as not all transactions might have been propagated by the network to us yet.
// Note that this._pendingTransactions could technically also include pending cashlink funding transactions
// which should not count towards pending claims but we can ignore this fact here.
|| this._pendingTransactions.length >= 5
) {
return Cashlink.MIN_PAID_TRANSACTION_FEE;
}
return 0;
}

get message() {
@@ -150,6 +171,7 @@ class Cashlink {
private _theme: CashlinkTheme = CashlinkTheme.UNSPECIFIED; // note that UNSPECIFIED equals to 0 and is thus falsy
private _getUserAddresses: () => Set<string>;
private _knownTransactions: DetailedPlainTransaction[] = [];
private _pendingTransactions: Array<Partial<DetailedPlainTransaction>> = [];

constructor(
public keyPair: Nimiq.KeyPair,
@@ -215,10 +237,10 @@ class Cashlink {
await this._awaitConsensus();

const balance = await this._awaitBalance(); // balance with pending outgoing transactions subtracted by nano-api
const pendingTransactions = [
this._pendingTransactions = [
...(await this._getNetwork()).pendingTransactions,
...(await this._getNetwork()).relayedTransactions,
];
].filter((tx) => tx.sender === cashlinkAddress || tx.recipient === cashlinkAddress);

const cashlinkAddress = this.address.toUserFriendlyAddress();
const userAddresses = this._getUserAddresses();
@@ -227,9 +249,9 @@ class Cashlink {
// as cashlink shouldn't be in CLAIMING state for other users' transactions. Instead, for us the state should
// remain UNCLAIMED if additional balance remains or directly switch to CLAIMED if there is no balance left for
// us, subtracting other users' pending claims.
const pendingFundingTx = pendingTransactions.find(
const pendingFundingTx = this._pendingTransactions.find(
(tx) => tx.recipient === cashlinkAddress);
const pendingClaimingTx = pendingTransactions.find(
const pendingClaimingTx = this._pendingTransactions.find(
(tx) => tx.sender === cashlinkAddress && userAddresses.has(tx.recipient!));

// Only exit if the cashlink is CLAIMED and not currently funded or being funded.
@@ -280,8 +302,12 @@ class Cashlink {
timestamp: this.timestamp,
};
if (includeOptional) {
result.fee = this.fee;
result.contactName = this.contactName;
if (this._fee !== null) {
result.fee = this._fee;
}
if (this.contactName) {
result.contactName = this.contactName;
}
}
return result;
}
@@ -327,7 +353,7 @@ class Cashlink {
layout: 'cashlink',
recipient: this.address,
value: this.value,
fee: this.fee,
fee: 0, // this.fee is meant for claiming transactions
data: CashlinkExtraData.FUNDING,
cashlinkMessage: this.message,
};
@@ -336,22 +362,27 @@ class Cashlink {
public async claim(
recipientAddress: string,
recipientType: Nimiq.Account.Type = Nimiq.Account.Type.BASIC,
fee: number = this.fee,
fee?: number,
): Promise<void> {
await Promise.all([
loadNimiq(),
this.state === CashlinkState.UNKNOWN ? this.detectState() : Promise.resolve(),
]);
if (this.state >= CashlinkState.CLAIMING) {
throw new Error('Cannot claim, Cashlink has already been claimed');
}

await loadNimiq();

// Get latest balance and fee
const balance = await this._awaitBalance();
fee = fee !== undefined ? fee : this.fee;
// Only claim the amount specified in the cashlink (or the cashlink balance, if smaller)
const balance = Math.min(this.value, await this._awaitBalance());
if (!balance) {
throw new Error('Cannot claim, there is no balance in this link');
const totalClaimBalance = Math.min(this.value + fee, balance);
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion to rename to totalClaimAmount, to signal that this has nothing to do with the cashlink's balance, but the claiming amount.

if (totalClaimBalance <= fee) {
throw new Error('Cannot claim, there is not enough balance in this link');
}
const recipient = Nimiq.Address.fromString(recipientAddress);
const transaction = new Nimiq.ExtendedTransaction(this.address, Nimiq.Account.Type.BASIC,
recipient, recipientType, balance - fee, fee, await this._getBlockchainHeight(),
recipient, recipientType, totalClaimBalance - fee, fee, await this._getBlockchainHeight(),
Nimiq.Transaction.Flag.NONE, CashlinkExtraData.CLAIMING);

const keyPair = this.keyPair;
@@ -476,6 +507,10 @@ namespace Cashlink {
export const DEFAULT_THEME = Date.now() < new Date('Tue, 13 Apr 2020 23:59:00 GMT-12').valueOf()
? CashlinkTheme.EASTER
: CashlinkTheme.STANDARD;

// (size of extended tx + cashlink extra data) * Nimiq.Mempool.TRANSACTION_RELAY_FEE_MIN which is 1. We can't use
// TRANSACTION_RELAY_FEE_MIN as constant because Mempool is not included in Nimiq core offline build.
export const MIN_PAID_TRANSACTION_FEE = 171;
}

export default Cashlink;
2 changes: 1 addition & 1 deletion tests/unit/CashlinkStore.spec.ts
Original file line number Diff line number Diff line change
@@ -27,7 +27,7 @@ const DUMMY_DATA = {
105, 128, 49, 54, 99, 159, 166, 103, 196, 208, 178, 26, 244, 184, 234,
]))).publicKey.toAddress(),
1234554321,
123,
171,
'Ein Cashlink test Cashlink',
CashlinkState.UNCLAIMED,
),