feat: new tx history - cache tx details #1371
Conversation
…rum-token-bridge into new-tx-history-cctp
…m-token-bridge into new-tx-history-ui-pagination
…m-token-bridge into new-tx-history-ui-pagination
| ? 'subgraph' | ||
| : 'event_logs', | ||
| timestampCreated: String(dayjs().valueOf() / 1_000), | ||
| nonce: tx.nonce |
There was a problem hiding this comment.
Requested by @fionnachan, adding nonce so that we can fix #986 in the future
| @@ -109,12 +112,17 @@ export function useClaimWithdrawal(): UseClaimWithdrawalResult { | |||
| } | |||
|
|
|||
| const isSuccess = (res as ContractReceipt).status === 1 | |||
| const txHash = (res as ContractReceipt).transactionHash | |||
| return ( | ||
| cachedTx.txID === tx.txID && | ||
| cachedTx.parentChainId === tx.parentChainId && | ||
| cachedTx.childChainId === tx.childChainId | ||
| ) |
There was a problem hiding this comment.
Use isSameTransaction after reducing the type to only these 3 params
| @@ -443,6 +471,30 @@ export const useArbTokenBridge = ( | |||
| childChainId: Number(l2NetworkID) | |||
| }) | |||
|
|
|||
| addDepositToCache({ | |||
| sender: walletAddress, | |||
| destination: walletAddress, | |||
| assetType: AssetType.ERC20, | ||
| l1NetworkID, | ||
| l2NetworkID, | ||
| value: utils.formatUnits(amount, nativeCurrency.decimals), |
| }) | ||
| ? 'subgraph' | ||
| : 'event_logs', | ||
| timestampCreated: String(dayjs().valueOf() / 1_000), |
| }) | ||
| ? 'subgraph' | ||
| : 'event_logs', | ||
| timestampCreated: String(dayjs().valueOf() / 1_000), |
| childChainId: Number(l2NetworkID), | ||
| direction: 'deposit', | ||
| type: 'deposit-l1', | ||
| source: subgraphExistsForChainPair({ |
There was a problem hiding this comment.
Let's add another value local_storage_cache
| (depositsData || []).flat(), | ||
| ...getDepositsWithoutStatusesFromCache() |
| numberOfDays: oldestTransactionDaysAgo | ||
| }, | ||
| loading: fetching, | ||
| completed: transactions.length === data.length, | ||
| completed: dedupedTransactions.length >= data.length, |
There was a problem hiding this comment.
Let's throw if deduped.length > data.length, and return to the previous implementation
There was a problem hiding this comment.
I moved deduping to getting txs without statuses, I don't think we need to throw anymore
| const dedupedTransactions = useMemo( | ||
| () => | ||
| Array.from( | ||
| new Map( | ||
| transactions.map(tx => [ | ||
| `${tx.parentChainId}-${tx.childChainId}-${getTxIdFromTransaction( | ||
| tx | ||
| )}}`, | ||
| tx | ||
| ]) | ||
| ).values() | ||
| ).sort(sortByTimestampDescending), | ||
| [transactions] | ||
| ) |
There was a problem hiding this comment.
use Set instead?
| const dedupedTransactions = useMemo( | |
| () => | |
| Array.from( | |
| new Map( | |
| transactions.map(tx => [ | |
| `${tx.parentChainId}-${tx.childChainId}-${getTxIdFromTransaction( | |
| tx | |
| )}}`, | |
| tx | |
| ]) | |
| ).values() | |
| ).sort(sortByTimestampDescending), | |
| [transactions] | |
| ) | |
| const dedupedTransactions = useMemo( | |
| () => Array.from(new Set(transactions)).sort(sortByTimestampDescending), | |
| [transactions] | |
| ) |
There was a problem hiding this comment.
I think we may want to stick with Map. Set dedupes by reference and we have unrelated objects with the same data so we could end up with duplicates.
There was a problem hiding this comment.
as discussed on Slack, we should keep the Map implementation but leave a comment above to ensure it doesn't accidentally get refactored to Set
| import { getWagmiChain } from '../../util/wagmi/getWagmiChain' | ||
| import { getL1ToL2MessageDataFromL1TxHash } from '../../util/deposits/helpers' | ||
| import { AssetType } from '../../hooks/arbTokenBridge.types' | ||
| import { getDepositStatus } from '../../state/app/utils' | ||
| import { getBlockBeforeConfirmation } from '../../state/cctpState' | ||
| import { getAttestationHashAndMessageFromReceipt } from '../../util/cctp/getAttestationHashAndMessageFromReceipt' | ||
|
|
||
| const CLAIM_PARENT_CHAIN_DETAILS_LOCAL_STORAGE_KEY = |
There was a problem hiding this comment.
Nit: Can we name it better? Wasn't clear at first what it meant 🤔
There was a problem hiding this comment.
Something like PARENT_CHAIN_TX_DETAILS_OF_CLAIM_TX ?
| * @param {MergedTransaction} tx - Transaction that initiated the withdrawal (child chain transaction). | ||
| * @param {string} parentChainTxId - Transaction ID of the claim transaction (parent chain transaction ID). | ||
| */ | ||
| export function setWithdrawalClaimParentChainTxDetails( |
There was a problem hiding this comment.
NIT: setParentChainTxDetailsOfWithdrawalClaimTx and similarly in other function?
| @@ -188,6 +189,9 @@ export const useArbTokenBridge = ( | |||
| } | |||
|
|
|||
| const ethBridger = await EthBridger.fromProvider(l2.provider) | |||
| const parentChainBlockTimestamp = Math.floor( | |||
There was a problem hiding this comment.
Why is Math.floor needed here since it will never be a decimal
There was a problem hiding this comment.
hmm I was getting a decimal value for some reason before but might have been a mistake on my part, removing math floor
| Array.from( | ||
| new Map( | ||
| transactions.map(tx => [ | ||
| `${tx.parentChainId}-${tx.childChainId}-${getTxIdFromTransaction( |
There was a problem hiding this comment.
Dumb Q: while setting the tx-id key, have we thought about upper-lowercase considerations that might mismatch same keys while checking?
There was a problem hiding this comment.
fair point, added toLowerCase
| @@ -15,7 +15,7 @@ export type FetchWithdrawalsFromSubgraphResult = { | |||
| l2TxHash: string | |||
| l2BlockNum: string | |||
| direction: 'deposit' | 'withdrawal' | |||
| source: 'subgraph' | 'event_logs' | |||
| source: 'subgraph' | 'event_logs' | 'local_storage_cache' | |||
There was a problem hiding this comment.
NIT: Hmm not sure why we have the 3 different sources when the type is FetchWithdrawalsFromSubgraphResult - cleary a result from subgraph.
There was a problem hiding this comment.
you're right, updated to 'subgraph' only
Previous PR: #1369 | Next PR: #1372