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: adding TransactionTimeToConfirmation event for confirmations #9

Closed
wants to merge 1 commit into from

Conversation

ryanml
Copy link
Contributor

@ryanml ryanml commented Jul 31, 2024

Description

Described in issue: https://ava-labs.atlassian.net/browse/CP-8783

Changes

Added new tracking event called TransactionTimeToConfirmation for successful token/nft sends, swaps, and bridge. There are no functional changes to the extension

Testing

  1. Do a smoke test of the transaction flows
  2. To confirm the tx event, add the following snippet to the captureTime function while testing:
console.log('Capturing TransactionTimeToConfirmation', {
  chainId: chainId,
  time: measurement.duration,
  txOrigin: txType,
});

Checklist for the author

Tick each of them when done or if not applicable.

  • I've covered new/modified business logic with Jest test cases.
  • I've tested the changes myself before sending it to code review and QA.

Copy link
Member

@meeh0w meeh0w left a comment

Choose a reason for hiding this comment

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

I may be wrong here, or the task description may be misleading me, but unfortunately I think we're measuring the wrong thing here. The task says:

...I want to track and improve on the perceived time to FIRST CONFIRMATION by the user in our apps.

Measure the time between the user approving the transaction and RECEIVING THE CONFIRMATION from the network

but what's being measured here is the time between these two events instead:

  • user approves the transaction
  • the transaction request is sent to the network

The transaction being "sent" is different than it being "confirmed".
"Sent" essentially means that you successfully sent the request, however the transaction could be rejected by the network (i.e. due to insufficient funds) later on, when validators try to include it in the blockchain (which may take some time depending on the network load and the fee you were willing to pay).

We already do track a "transaction confirmed" event for Swap here:

https://github.com/ava-labs/extension-avalanche/blob/c4618478eb0749aabe8aa08a960271e997bd3e37/src/contexts/SwapProvider/SwapProvider.tsx#L452-L459

However, it's done on the frontend at the moment which isn't great - I think it should be done on the background script. My reasoning:

  1. All of the features the current implementation measures (Bridge, CollectibleSend, Send, SignTransaction and Swap) trigger one of the three RPC calls:

    • eth_sendTransaction
    • bitcoin_sendTransaction
    • avalanche_sendTransaction

    They all have separate handlers (with the same name) which implement onActionApproved method. This gives us very nice places to plug the measurements into and those measurements would then happen for every feature, no matter if they are triggered by the extension frontend or a dApp.

  2. The time between sending a transaction and it being confirmed can be a few seconds (Avalanche), ~30s (Ethereum) and also much longer (Bitcoin). With that in mind - when user approves the in-app transaction, they are free to close the extension popup and then nothing will get reported (popup gets closed -> frontend script does not execute anymore).

  3. When user approves the dApp-originated transaction (i.e. a request coming from core.app), the popup gets closed as soon as the transaction is sent and so nothing will get reported.

    • On that note, the requests coming from dApps are not included in the current implementation too.

For some tips, here's how Core Web team did it:

@ryanml
Copy link
Contributor Author

ryanml commented Aug 5, 2024

I've refactored this to move the tracking from the UI where it was originally to the background. This measures the time from approval to confirmation (not just sent like previously) I have two open questions that should hopefully be quickly resolveable:

  1. I am not sure of the best way to get the transaction type from the context on onActionApproved in the send handlers
  2. Though the ETH and BTC providers had a "wait for transaction" method to get the confirmation, I did not see a similar method implemented for the Avalanche provider in its handler. What is the appropriate way to wait for confirmation there?

Copy link
Contributor

@bferenc bferenc left a comment

Choose a reason for hiding this comment

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

I am not sure of the best way to get the transaction type from the context on onActionApproved in the send handlers

We can add that info to the displayOptions param IMO. If the info is missing, the request is not coming from our UI, but from a dApp in which case the transactionType should be RPC according to the ticket.

Though the ETH and BTC providers had a "wait for transaction" method to get the confirmation, I did not see a similar method implemented for the Avalanche provider in its handler. What is the appropriate way to wait for confirmation there?

Avalanche transactions are tricky, let's handle their measurement in a different task.

Question / request:

  • what happens if a user has multiple pending transactions?
  • please also chain the catch method to those promises so we can handle rejections gracefully
  • please update the tests and check if the analytics events are constructed / sent properly

Comment on lines +277 to +287
measureTransactionTime().endMeasure(async (duration) => {
this.analyticsServicePosthog.captureEvent({
name: 'TransactionTimeToConfirmation',
windowId: crypto.randomUUID(),
properties: {
duration,
txType: 'txType',
chainId: usedNetwork,
},
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

🔥

Suggested change
measureTransactionTime().endMeasure(async (duration) => {
this.analyticsServicePosthog.captureEvent({
name: 'TransactionTimeToConfirmation',
windowId: crypto.randomUUID(),
properties: {
duration,
txType: 'txType',
chainId: usedNetwork,
},
});
});

@@ -231,6 +232,7 @@ export class AvalancheSendTransactionHandler extends DAppRequestHandler<
const usedNetwork = this.#getChainIdForVM(vm);

try {
measureTransactionTime().startMeasure();
Copy link
Contributor

Choose a reason for hiding this comment

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

🔥

Suggested change
measureTransactionTime().startMeasure();

@@ -24,6 +24,7 @@ import getProvidedUtxos from '../utils/getProvidedUtxos';
import { AnalyticsServicePosthog } from '../../analytics/AnalyticsServicePosthog';
import { ChainId } from '@avalabs/core-chains-sdk';
import { openApprovalWindow } from '@src/background/runtime/openApprovalWindow';
import { measureTransactionTime } from '@src/background/services/wallet/utils/measureTransactionTime';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import { measureTransactionTime } from '@src/background/services/wallet/utils/measureTransactionTime';

TRANSACTION_APPROVED = 'transaction-approved',
}

export const measureTransactionTime = (): {
Copy link
Contributor

Choose a reason for hiding this comment

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

While this util is handy, it is introducing some issues when there are multiple concurrent transactions waiting for confirmations. Since you are using the same marks for all measurements, the start time can be overwritten or cleared by an upcoming transaction.

@ryanml ryanml closed this Aug 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants