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

Don't detect pending transactions if no nonce in 20 latest transactions #2022

Merged
merged 4 commits into from
Jul 30, 2024
Merged
Show file tree
Hide file tree
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
1 change: 1 addition & 0 deletions .changelog/2022.bugfix.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Don't detect pending transactions if no nonce in 20 latest transactions
2 changes: 1 addition & 1 deletion src/app/state/account/saga.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@ import { getAccountBalanceWithFallback } from '../../lib/getAccountBalanceWithFa
import { walletActions } from '../wallet'
import { selectSelectedNetwork } from '../network/selectors'
import { Transaction } from '../transaction/types'
import { TRANSACTIONS_LIMIT } from '../../../config'

const ACCOUNT_REFETCHING_INTERVAL = process.env.REACT_APP_E2E_TEST ? 5 * 1000 : 10 * 1000
const TRANSACTIONS_LIMIT = 20

export function* fetchAccount(action: PayloadAction<string>) {
const address = action.payload
Expand Down
171 changes: 171 additions & 0 deletions src/app/state/account/selectors.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,171 @@
import { TRANSACTIONS_LIMIT } from '../../../config'
import { RootState } from '../../../types'
import { Transaction, TransactionStatus } from '../transaction/types'
import { hasAccountUnknownPendingTransactions } from './selectors'

const currentAddress = 'oasis1qz78ap0456g2rk7j6rmtvasc9v2kjhz2s58qgj90'

describe('hasAccountUnknownPendingTransactions', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some tests seem like duplication of src/app/pages/AccountPage/Features/TransactionHistory/__tests__/index.test.tsx, but since they are unit tests, looks fine.

it('empty account', () => {
expect(
hasAccountUnknownPendingTransactions({
account: {
address: currentAddress,
nonce: 0n.toString(),
transactions: [] as Transaction[],
},
} as RootState),
).toBe(false)
})

it('making first transaction', () => {
expect(
hasAccountUnknownPendingTransactions({
account: {
address: currentAddress,
nonce: 1n.toString(),
transactions: [] as Transaction[],
},
} as RootState),
).toBe(true)

expect(
hasAccountUnknownPendingTransactions({
account: {
address: currentAddress,
nonce: 1n.toString(),
transactions: [
{
from: 'oasis1qp2frld759c6u92pxs768nd2ctnrduhqp5f6f273',
to: currentAddress, // Received
status: TransactionStatus.Successful,
nonce: 0n.toString(),
},
] as Transaction[],
},
} as RootState),
).toBe(true)

expect(
hasAccountUnknownPendingTransactions({
account: {
address: currentAddress,
nonce: 1n.toString(),
transactions: [
{
from: currentAddress, // Sent
to: 'oasis1qp2frld759c6u92pxs768nd2ctnrduhqp5f6f273',
status: TransactionStatus.Successful,
nonce: 0n.toString(),
},
] as Transaction[],
},
} as RootState),
).toBe(false)
})

it('failed transaction also updates account nonce in consensus', () => {
expect(
hasAccountUnknownPendingTransactions({
account: {
address: currentAddress,
nonce: 1n.toString(),
transactions: [
{
from: currentAddress,
to: 'oasis1qp2frld759c6u92pxs768nd2ctnrduhqp5f6f273',
status: TransactionStatus.Failed,
nonce: 0n.toString(),
},
] as Transaction[],
},
} as RootState),
).toBe(false)
})

it('multiple pending transactions', () => {
expect(
hasAccountUnknownPendingTransactions({
account: {
address: currentAddress,
nonce: 5n.toString(),
transactions: [
{
from: currentAddress, // Sent
to: 'oasis1qp2frld759c6u92pxs768nd2ctnrduhqp5f6f273',
status: TransactionStatus.Successful,
nonce: 0n.toString(),
},
] as Transaction[],
},
} as RootState),
).toBe(true)
})

it('paratime transaction updates a different nonce within the paratime', () => {
expect(
hasAccountUnknownPendingTransactions({
account: {
address: currentAddress,
nonce: 1n.toString(),
transactions: [
{
from: currentAddress, // Sent on Emerald
to: 'oasis1qq235lqj77855qcemcr5w2qm372s4amqcc4v3ztc', // 0xC3ecf872F643C6238Aa20673798eed6F7dA199e9
status: TransactionStatus.Successful,
runtimeName: 'Emerald',
runtimeId: '000000000000000000000000000000000000000000000000e2eaa99fc008f87f',
round: 11129138,
type: 'consensus.Deposit',
nonce: 0n.toString(), // Misleading Emerald nonce
},
] as Transaction[],
},
} as RootState),
).toBe(true)
})

it('multiple pages of transactions', () => {
const onePageOfReceivedTxs = new Array(TRANSACTIONS_LIMIT).fill(null).map(() => ({
from: 'oasis1qp2frld759c6u92pxs768nd2ctnrduhqp5f6f273',
to: currentAddress, // Received
status: TransactionStatus.Successful,
nonce: 999n.toString(),
})) as Transaction[]
expect(
hasAccountUnknownPendingTransactions({
account: {
address: currentAddress,
nonce: 101n.toString(),
transactions: onePageOfReceivedTxs,
},
} as RootState),
).toBe(false) // Can't determine within first page. Assume false.

const onePageIncludingSentTx = [...onePageOfReceivedTxs]
onePageIncludingSentTx[4] = {
from: currentAddress, // Sent
to: 'oasis1qp2frld759c6u92pxs768nd2ctnrduhqp5f6f273',
status: TransactionStatus.Successful,
nonce: 100n.toString(),
} as Transaction
expect(
hasAccountUnknownPendingTransactions({
account: {
address: currentAddress,
nonce: 101n.toString(),
transactions: onePageIncludingSentTx,
},
} as RootState),
).toBe(false)
expect(
hasAccountUnknownPendingTransactions({
account: {
address: currentAddress,
nonce: 102n.toString(),
transactions: onePageIncludingSentTx,
},
} as RootState),
).toBe(true)
})
})
38 changes: 14 additions & 24 deletions src/app/state/account/selectors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { createSelector } from '@reduxjs/toolkit'
import { RootState } from 'types'
import { initialState } from '.'
import { selectSelectedNetwork } from '../network/selectors'
import { TRANSACTIONS_LIMIT } from '../../../config'

const selectSlice = (state: RootState) => state.account || initialState

Expand All @@ -25,32 +26,21 @@ export const selectPendingTransactionForAccount = createSelector(
export const hasAccountUnknownPendingTransactions = createSelector(
[selectAccountNonce, selectTransactions, selectAccountAddress],
(accountNonce, transactions, accountAddress) => {
// TODO: This logic fails, in case transaction are not from the initial page of account's transactions
const filteredTransactions = transactions.filter(({ from }) => from && from === accountAddress)

if (filteredTransactions.length === 0 && BigInt(accountNonce) === 0n) return false

const maxNonceFromTxs = filteredTransactions.reduce(
(acc, { nonce }) => {
if (!nonce) {
return acc
}

const bigNonce = BigInt(nonce)

if (!acc) {
return bigNonce
}

return bigNonce > acc ? bigNonce : acc
},
undefined as bigint | undefined,
)

if (maxNonceFromTxs === undefined) {
const noncesFromTxs = transactions
.filter(tx => !tx.runtimeId)
.filter(tx => tx.from !== undefined)
.filter(tx => tx.from === accountAddress)
.filter(tx => tx.nonce !== undefined)
.map(tx => BigInt(tx.nonce!))

if (noncesFromTxs.length <= 0) {
// TODO: last transaction that affected nonce is not in the initial page of account's transactions
if (transactions.length >= TRANSACTIONS_LIMIT) return false
// Waiting for first transactions
return BigInt(accountNonce) > 0n
}

return BigInt(maxNonceFromTxs) + BigInt(1) < BigInt(accountNonce)
const maxNonceFromTxs = noncesFromTxs.reduce((acc, nonce) => (nonce > acc ? nonce : acc))
return BigInt(accountNonce) > maxNonceFromTxs + 1n
},
)
2 changes: 2 additions & 0 deletions src/config.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import { NetworkType } from 'app/state/network/types'

export const TRANSACTIONS_LIMIT = 20

export const consensusDecimals = 9

// Moved outside backend.ts to avoid circular dependency
Expand Down
2 changes: 1 addition & 1 deletion src/vendors/__tests__/__snapshots__/oasisscan.test.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ exports[`oasisscan parse transaction list 1`] = `
"from": "oasis1qqnk4au603zs94k0d0n7c0hkx8t4p6r87s60axru",
"hash": "25b84ca4582f6e3140c384ad60b98415d2c3079d1fc5f2221e45da7b13c70817",
"level": undefined,
"nonce": undefined,
"nonce": "4",
"round": 997775,
"runtimeId": "000000000000000000000000000000000000000000000000e2eaa99fc008f87f",
"runtimeName": "Emerald",
Expand Down
3 changes: 2 additions & 1 deletion src/vendors/oasisscan.ts
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,8 @@ export function parseTransactionsList(
runtimeName: t.runtimeName,
runtimeId: t.runtimeId,
round: t.round,
nonce: undefined,
nonce:
t.ctx?.nonce ?? t.etx?.nonce != null ? BigInt(t.ctx?.nonce ?? t.etx?.nonce).toString() : undefined,
}
return parsed
} else {
Expand Down
Loading