(Multi-)Cashlink improvements #489

danimoh · 2022-06-08T10:37:10Z

Improvements to cashlinks and especially multi-claimable cashlinks.
See commit history for details.

I'm also considering two additional changes:

to split the Cashlink class into a Cashlink base class and an InteractiveCashlink class because half to the Cashlink's methods can only be used after the network (and now also user addresses) have been set as dependencies. These methods are only needed for state observation and claiming the cashlink, and thus only in CashlinkReceive which would be the only place where we would need the InteractiveCashlink. In all other places and especially the CashlinkStore we would just use the base Cashlink class which would be safe to exist without the additional dependencies. An InteractiveCashlink can be created from a base Cashlink by passing the base cashlink instance and the additional dependencies in the constructor. This would reduce the base Cashlink's code and improve type safety as the methods that have the additional dependencies would only exist on instances of InteractiveCashlink.
To not store the cashlink state anymore as it doesn't really fulfill a purpose imo and just takes unnecessary space.

Let me know what you think.

Also now in theory more resilient to very long cashlink tx histories that can not be retrieved entirely from nodes such that knownFundingTx is not included anymore. In this case the detection is now able to at least reach CHARGED or CLAIMING (which is also displayed in the ui similarly to CLAIMED) states.

…sers For pending claiming transactions, only consider our transactions 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. This better distinction of states also allowed for handling the CLAIMING state separately in the CashlinkReceive UI.

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.

Previously, repeat claims were detected only by the logged in addresses. Now also storing claimed multi-cashlinks in localStorage for the case that user logged out of his accounts.

If we can exit early, we don't have to query the transaction history, which is a costly operation.

- Avoid double invocations from events triggered at the same time. Instead, wait a little bit for all events and their data to hopefully have processed on the network side, such that we don't work with outdated or inaccurate data. - Update our local variables again after fetching the transaction history finished to avoid potentially working with outdated data. - Harden detection of empty cashlink as CLAIMED by checking whether we know at least one claiming tx instead of relying only on empty balance to avoid CLAIMED state after funding if knownFundingTx is already known but balance update was not triggered yet.

Leads to little code simplification and now also covers handling expired transactions.

For multi-claimable cashlinks which have less than the denominated cashlink value left, display only what the user can actually still receive.

sisou · 2022-06-09T08:23:37Z

src/lib/Cashlink.ts

+        try {
+            return JSON.parse(window.localStorage[Cashlink.LAST_CLAIMED_MULTI_CASHLINKS_STORAGE_KEY])
+                .map((addressBase64: string) => Nimiq.Address.fromBase64(addressBase64));
+        } catch (e) {
+            return [];
+        }


I understand that this just nicely fails when the key does not exist in localstorage (null can not be .maped), but I would think it's cleaner to check for the key's existence first and only then try to parse it's JSON.

sisou · 2022-06-09T08:35:12Z

src/lib/Cashlink.ts

+        [this._pendingTransactions, pendingFundingTx, ourPendingClaimingTx] = await this._getPendingTransactions();
+        let ourKnownClaimingTx = this._knownTransactions.find( // for now based on old known transactions
+            (tx) => tx.sender === cashlinkAddress && userAddresses.has(tx.recipient));


What are the differences between pendingTransactions & knownTransactions, and ourPendingClaimingTx & ourKnownClaimingTx? Aren't those the same, respectively?

sisou · 2022-06-09T08:41:55Z

src/lib/Cashlink.ts

-        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);


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

sisou · 2022-06-09T08:43:01Z

src/lib/Cashlink.ts


-        transaction.proof = proof;
+        if (balance > this.value) {


I think this check (if it's a multi-cashlink) should include the fee in the check, as otherwise single-cashlinks that include the claiming fee would also be characterized as multi-cashlinks.

Suggested change

if (balance > this.value) {

if (balance > this.value + fee) {

sisou · 2022-06-09T08:48:53Z

src/lib/Cashlink.ts

+        const pendingTransactions = [
+            ...network.pendingTransactions,
+            ...network.relayedTransactions,
+        ].filter((tx) => tx.sender === cashlinkAddress || tx.recipient === cashlinkAddress);
+
+        const pendingFundingTx = this._pendingTransactions.find(
+            (tx) => tx.recipient === cashlinkAddress);
+        const ourPendingClaimingTx = this._pendingTransactions.find(
+            (tx) => tx.sender === cashlinkAddress && userAddresses.has(tx.recipient!));


You are creating pendingTransactions from the network client, but then are using this._pendingTransactions to filter for funding/claiming txs. Is that on purpose? Is this._pendingTransactions more up-to-date than the just-created pendingTransactions?

Checked the file again. I think this is a mistake. this._pendingTransactions is only updated when this function returns, so I think lines 542 and 544 should use the just-created pendingTransactions, otherwise they are filtering on outdated data.

sisou · 2022-06-09T08:56:42Z

src/views/CashlinkCreate.vue

@@ -160,7 +160,7 @@ class CashlinkCreate extends Vue {
        }, {
            color: 'nq-green-bg',
            value: 1,
-            get text() { return i18n.t('standard') as string; },
+            get text() { return i18n.t('standard') as string; },


Why do different versions of a space even exist? :D

sisou · 2022-06-09T08:57:51Z

src/views/CashlinkReceive.vue

+                            :amount="cashlink.balance
+                                ? Math.min(cashlink.value, cashlink.balance - cashlink.fee)
+                                : cashlink.value"


I feel this should be a getter on the cashlink class. The cashlink should know how much it can be claimed for.
E.g. cashlink.claimAmount.

sisou · 2022-06-09T08:58:56Z

src/views/CashlinkReceive.vue

+                        :amount="cashlink.balance
+                            ? Math.min(cashlink.value, cashlink.balance - cashlink.fee)
+                            : cashlink.value"


Same here. This feels like cashlink-internal business logic.

sisou · 2022-06-09T09:00:28Z

src/views/CashlinkReceive.vue

+        if (this.cashlink!.state !== CashlinkState.CLAIMED && this.statusState === StatusScreen.State.WARNING) {
+            // Reset warning screen if cashlink state reverted from CLAIMING state, e.g. by being refunded.
+            this.statusState = false;
+        }


The condition checks for CLAIMED state, but the comment talks about CLAIMING state. Mistake or on purpose and needs clarification?

sisou · 2022-06-09T14:26:11Z

Regarding the additional changes:

I am very in favor of this. Makes a lot of sense to me! Would also remove the need to use ! to force non-null types in most places I believe.
I don't quite understand. You mean to not store cashlinks anymore? Or only not store their state/status?

danimoh added 11 commits May 29, 2022 12:22

Cashlink: disallow repeat claiming of multi-cashlinks

a8898e1

Cashlink: detect repeat claims also by remembering claimed cashlinks

ca82b26

Previously, repeat claims were detected only by the logged in addresses. Now also storing claimed multi-cashlinks in localStorage for the case that user logged out of his accounts.

Cashlink: more early exit conditions in detectState

2f8f14a

If we can exit early, we don't have to query the transaction history, which is a costly operation.

Cashlink: use new network api for transactions events

a5d52d0

Leads to little code simplification and now also covers handling expired transactions.

CashlinkReceive: cap displayed amount to what user can receive

4551d26

For multi-claimable cashlinks which have less than the denominated cashlink value left, display only what the user can actually still receive.

CashlinkReceive: correctly handle reverting from CLAIMED state

7c08bc2

CashlinkReceive: don't show warning screen if we claimed ourselves

9dfebd7

danimoh requested a review from sisou June 8, 2022 10:37

danimoh self-assigned this Jun 8, 2022

sisou requested changes Jun 9, 2022

View reviewed changes

danimoh force-pushed the master branch from ad0694c to 28eba60 Compare July 8, 2024 17:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(Multi-)Cashlink improvements #489

(Multi-)Cashlink improvements #489

danimoh commented Jun 8, 2022

sisou Jun 9, 2022

sisou Jun 9, 2022

sisou Jun 9, 2022

sisou Jun 9, 2022

sisou Jun 9, 2022

sisou Jun 9, 2022

sisou Jun 9, 2022

sisou Jun 9, 2022

sisou Jun 9, 2022

sisou Jun 9, 2022

sisou commented Jun 9, 2022

	if (balance > this.value) {
	if (balance > this.value + fee) {

(Multi-)Cashlink improvements #489

Are you sure you want to change the base?

(Multi-)Cashlink improvements #489

Conversation

danimoh commented Jun 8, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sisou commented Jun 9, 2022