feat: remove local storage txn logic and move to new bridge api architecture #116
Conversation
- Replace bridge-ui-indexer with direct bridge API integration - Remove localStorage transaction storage - Use POST /v2/transaction/:tx_hash to mark transactions as initiated - Add 20-second auto-polling for transaction updates - Consolidate all transaction fetching into unified flow - Fix typo: detinationBlockhash → destinationBlockhash
The bridge API doesn't provide timestamp data which causes: - Incorrect transaction sorting (all show same time) - Poor UX in transaction list (displays 'just now' for all) Needs API team to add timestamp fields or implement blockchain fetch
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile OverviewGreptile SummaryThis PR migrates from a localStorage-based transaction system with an indexer service to a new Bridge API architecture. The changes simplify transaction management by removing complex local/remote merging logic and relying on the API as the source of truth. Key ChangesArchitecture Migration:
Transaction Flow:
Component Updates:
Critical Issues Found🔴 Blocking Issues
🟡 Type Safety Issues
Confidence Score: 1/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant User
participant Container
participant useTransactions
participant TransactionsStore
participant BridgeAPI
participant LiquidityAPI
participant WormholeAPI
User->>Container: Connect wallet
Container->>TransactionsStore: fetchAllTransactions (initial)
Container->>Container: Start 15s polling interval
useTransactions->>TransactionsStore: startPolling()
TransactionsStore->>TransactionsStore: Start 20s polling interval
Note over Container,TransactionsStore: ⚠️ DUPLICATE POLLING<br/>Both intervals running!
loop Every 15s (Container)
Container->>TransactionsStore: fetchAllTransactions
end
loop Every 20s (Store)
TransactionsStore->>TransactionsStore: fetchAllTransactions
end
TransactionsStore->>BridgeAPI: GET /transactions
BridgeAPI-->>TransactionsStore: transactions (❌ no timestamp!)
Note over TransactionsStore,BridgeAPI: Sets sourceTimestamp = new Date()<br/>All txns appear as "just now"
TransactionsStore->>LiquidityAPI: fetchAllLiquidityBridgeTransactions
LiquidityAPI-->>TransactionsStore: transactions with timestamps
TransactionsStore->>WormholeAPI: fetchWormholeTransactions
WormholeAPI-->>TransactionsStore: transactions
TransactionsStore->>TransactionsStore: Deduplicate by hash
TransactionsStore->>TransactionsStore: Set pendingIndexedTransactions
useTransactions->>TransactionsStore: Detect pendingIndexedTransactions
useTransactions->>TransactionsStore: setIndexedTransactions
useTransactions->>TransactionsStore: setPendingIndexedTransactions(null)
useTransactions->>useTransactions: Filter & sort transactions
Note over useTransactions: ❌ Sort breaks because<br/>Bridge API txns have same time
useTransactions-->>User: Display transactions
User->>User: Switch account
useTransactions->>TransactionsStore: stopPolling()
useTransactions->>TransactionsStore: startPolling(newAddresses)
Note over TransactionsStore: ❌ Stale closure!<br/>Still fetches old addresses
User->>User: Initiate new transaction
useTransactions->>BridgeAPI: POST /v2/transaction/:txHash
Note over useTransactions,BridgeAPI: Mark transaction as initiated
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const intervalId = setInterval(() => { | ||
| const { setTransactionLoader } = get(); | ||
| get().fetchAllTransactions({ | ||
| ethAddress, | ||
| availAddress, | ||
| setTransactionLoader, | ||
| isInitialFetch: false, | ||
| }); | ||
| }, 20000); |
There was a problem hiding this comment.
The polling interval captures ethAddress and availAddress in a closure, which means these values will become stale and won't update when the user switches accounts. If a user switches from account A to account B, the polling will continue to fetch transactions for account A.
To fix this, the interval callback should read the current addresses from the store or component state dynamically rather than relying on the closure:
| const intervalId = setInterval(() => { | |
| const { setTransactionLoader } = get(); | |
| get().fetchAllTransactions({ | |
| ethAddress, | |
| availAddress, | |
| setTransactionLoader, | |
| isInitialFetch: false, | |
| }); | |
| }, 20000); | |
| const intervalId = setInterval(() => { | |
| const { setTransactionLoader, pollingIntervalId } = get(); | |
| // Don't fetch if we've been stopped | |
| if (!pollingIntervalId) return; | |
| // Get current addresses from params stored in state or pass them dynamically | |
| get().fetchAllTransactions({ | |
| ethAddress, | |
| availAddress, | |
| setTransactionLoader, | |
| isInitialFetch: false, | |
| }); | |
| }, 20000); |
However, a better solution would be to store the addresses in the Zustand state and have startPolling restart when addresses change, or pass a getter function that returns current addresses.
Prompt To Fix With AI
This is a comment left during a code review.
Path: stores/transactions.ts
Line: 135:143
Comment:
The polling interval captures `ethAddress` and `availAddress` in a closure, which means these values will become stale and won't update when the user switches accounts. If a user switches from account A to account B, the polling will continue to fetch transactions for account A.
To fix this, the interval callback should read the current addresses from the store or component state dynamically rather than relying on the closure:
```suggestion
const intervalId = setInterval(() => {
const { setTransactionLoader, pollingIntervalId } = get();
// Don't fetch if we've been stopped
if (!pollingIntervalId) return;
// Get current addresses from params stored in state or pass them dynamically
get().fetchAllTransactions({
ethAddress,
availAddress,
setTransactionLoader,
isInitialFetch: false,
});
}, 20000);
```
However, a better solution would be to store the addresses in the Zustand state and have `startPolling` restart when addresses change, or pass a getter function that returns current addresses.
How can I resolve this? If you propose a fix, please make it concise.| // TODO: Bridge API doesn't provide sourceTimestamp (created_at/timestamp field). | ||
| // This causes issues with: | ||
| // 1. Transaction sorting - all transactions appear to have the same time | ||
| // 2. UI display - shows "just now" for all transactions instead of actual time | ||
| // Solution: Request API team to add timestamp fields or fetch from blockchain | ||
| sourceTimestamp: new Date().toISOString(), | ||
| }; |
There was a problem hiding this comment.
Setting sourceTimestamp: new Date().toISOString() creates a critical bug. This causes ALL transactions from the Bridge API to appear with the current timestamp instead of their actual creation time. The TODO comment acknowledges this but the implementation is severely broken:
- All transactions show "just now" - Users can't see when transactions actually occurred
- Sorting is broken - Transactions can't be sorted by actual time since they all have the same (current) timestamp
- Type inconsistency -
sourceTimestampis defined asstringin types/transaction.ts (line 92), but other parts of the codebase set it to a number via.getTime()(see bridgeapi.ts lines 213, 266)
This is a blocking issue. The Bridge API must provide timestamp data, or you need to fetch it from the blockchain. Using new Date().toISOString() is not an acceptable workaround as it breaks core functionality.
Prompt To Fix With AI
This is a comment left during a code review.
Path: services/transactions.ts
Line: 100:106
Comment:
Setting `sourceTimestamp: new Date().toISOString()` creates a critical bug. This causes ALL transactions from the Bridge API to appear with the current timestamp instead of their actual creation time. The TODO comment acknowledges this but the implementation is severely broken:
1. **All transactions show "just now"** - Users can't see when transactions actually occurred
2. **Sorting is broken** - Transactions can't be sorted by actual time since they all have the same (current) timestamp
3. **Type inconsistency** - `sourceTimestamp` is defined as `string` in types/transaction.ts (line 92), but other parts of the codebase set it to a number via `.getTime()` (see bridgeapi.ts lines 213, 266)
This is a blocking issue. The Bridge API must provide timestamp data, or you need to fetch it from the blockchain. Using `new Date().toISOString()` is not an acceptable workaround as it breaks core functionality.
How can I resolve this? If you propose a fix, please make it concise.| ? encodeAddress(tx.receiver_hash) | ||
| : "0x0", | ||
| sourceTransactionHash: tx.tx_hash, | ||
| sourceTimestamp: new Date(tx.created_at).getTime(), |
There was a problem hiding this comment.
Type mismatch: Same issue as line 213 - sourceTimestamp is defined as string in types/transaction.ts but here you're assigning a number from .getTime().
| sourceTimestamp: new Date(tx.created_at).getTime(), | |
| sourceTimestamp: new Date(tx.created_at).toISOString(), |
Prompt To Fix With AI
This is a comment left during a code review.
Path: services/bridgeapi.ts
Line: 266:266
Comment:
Type mismatch: Same issue as line 213 - `sourceTimestamp` is defined as `string` in types/transaction.ts but here you're assigning a `number` from `.getTime()`.
```suggestion
sourceTimestamp: new Date(tx.created_at).toISOString(),
```
How can I resolve this? If you propose a fix, please make it concise.| useEffect(() => { | ||
| if (!selected?.address && !address) { | ||
| const accountAddress = selected?.address || address; | ||
|
|
||
| if (!accountAddress) { | ||
| stopPolling(); | ||
| return; | ||
| } | ||
|
|
||
| const loadLocalTransactions = async () => { | ||
| const accountAddress = selected?.address || address; | ||
| const storageKey = `localTransactions:${accountAddress}`; | ||
| const storedTxns = JSON.parse( | ||
| localStorage.getItem(storageKey) || "[]", | ||
| ) as Transaction[]; | ||
| startPolling({ | ||
| ethAddress: address, | ||
| availAddress: selected?.address, | ||
| }); | ||
|
|
||
| localTransactions.length = 0; | ||
| localTransactions.push(...storedTxns); | ||
| lastLocalTxnCountRef.current = storedTxns.length; | ||
| return () => { | ||
| stopPolling(); | ||
| }; | ||
|
|
||
| loadLocalTransactions(); | ||
| }, [selected?.address, address]); | ||
| }, [selected?.address, address, startPolling, stopPolling]); |
There was a problem hiding this comment.
The useEffect has startPolling and stopPolling in its dependency array, but these are Zustand store functions that have stable references. However, the effect will re-run every time selected?.address or address changes, which causes:
stopPolling()to be calledstartPolling()to be called with new addresses- Then the cleanup function runs
stopPolling()again
This creates a race condition where the polling might be stopped immediately after being started if the component re-renders. Additionally, calling startPolling on every address change is correct, but the cleanup always calling stopPolling means polling stops when the component unmounts even if the user is still logged in.
Consider removing startPolling and stopPolling from the dependency array since they're stable Zustand functions:
| useEffect(() => { | |
| if (!selected?.address && !address) { | |
| const accountAddress = selected?.address || address; | |
| if (!accountAddress) { | |
| stopPolling(); | |
| return; | |
| } | |
| const loadLocalTransactions = async () => { | |
| const accountAddress = selected?.address || address; | |
| const storageKey = `localTransactions:${accountAddress}`; | |
| const storedTxns = JSON.parse( | |
| localStorage.getItem(storageKey) || "[]", | |
| ) as Transaction[]; | |
| startPolling({ | |
| ethAddress: address, | |
| availAddress: selected?.address, | |
| }); | |
| localTransactions.length = 0; | |
| localTransactions.push(...storedTxns); | |
| lastLocalTxnCountRef.current = storedTxns.length; | |
| return () => { | |
| stopPolling(); | |
| }; | |
| loadLocalTransactions(); | |
| }, [selected?.address, address]); | |
| }, [selected?.address, address, startPolling, stopPolling]); | |
| }, [selected?.address, address]); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: hooks/useTransactions.ts
Line: 31:47
Comment:
The useEffect has `startPolling` and `stopPolling` in its dependency array, but these are Zustand store functions that have stable references. However, the effect will re-run every time `selected?.address` or `address` changes, which causes:
1. `stopPolling()` to be called
2. `startPolling()` to be called with new addresses
3. Then the cleanup function runs `stopPolling()` again
This creates a race condition where the polling might be stopped immediately after being started if the component re-renders. Additionally, calling `startPolling` on every address change is correct, but the cleanup always calling `stopPolling` means polling stops when the component unmounts even if the user is still logged in.
Consider removing `startPolling` and `stopPolling` from the dependency array since they're stable Zustand functions:
```suggestion
}, [selected?.address, address]);
```
How can I resolve this? If you propose a fix, please make it concise.| return apiTransactions.map((tx: any) => { | ||
| // Parse direction to get source and destination chains | ||
| const isEthToAvail = tx.direction === "EthAvail"; | ||
| const sourceChain = isEthToAvail ? Chain.ETH : Chain.AVAIL; | ||
| const destinationChain = isEthToAvail ? Chain.AVAIL : Chain.ETH; | ||
|
|
||
| // Map status from API to TransactionStatus enum | ||
| let status; | ||
| switch (tx.status) { | ||
| case "ClaimReady": | ||
| status = "READY_TO_CLAIM"; | ||
| break; | ||
| case "Bridged": | ||
| status = "CLAIMED"; | ||
| break; | ||
| case "Pending": | ||
| status = "PENDING"; | ||
| break; | ||
| default: | ||
| status = tx.status; | ||
| } |
There was a problem hiding this comment.
The status mapping has a critical flaw: the default case (line 82) returns tx.status directly without validation. If the API returns an unexpected status string, it will be assigned to the Transaction object even though it doesn't match any TransactionStatus enum value. This breaks type safety and could cause runtime errors in components that switch on transaction status.
For example, if the API returns "Processing" or "Initiated", these would be passed through unchanged and not handled properly by the UI.
| return apiTransactions.map((tx: any) => { | |
| // Parse direction to get source and destination chains | |
| const isEthToAvail = tx.direction === "EthAvail"; | |
| const sourceChain = isEthToAvail ? Chain.ETH : Chain.AVAIL; | |
| const destinationChain = isEthToAvail ? Chain.AVAIL : Chain.ETH; | |
| // Map status from API to TransactionStatus enum | |
| let status; | |
| switch (tx.status) { | |
| case "ClaimReady": | |
| status = "READY_TO_CLAIM"; | |
| break; | |
| case "Bridged": | |
| status = "CLAIMED"; | |
| break; | |
| case "Pending": | |
| status = "PENDING"; | |
| break; | |
| default: | |
| status = tx.status; | |
| } | |
| // Map status from API to TransactionStatus enum | |
| let status: TransactionStatus; | |
| switch (tx.status) { | |
| case "ClaimReady": | |
| status = TransactionStatus.READY_TO_CLAIM; | |
| break; | |
| case "Bridged": | |
| status = TransactionStatus.CLAIMED; | |
| break; | |
| case "Pending": | |
| status = TransactionStatus.PENDING; | |
| break; | |
| default: | |
| Logger.error(`Unknown transaction status from API: ${tx.status}`); | |
| status = TransactionStatus.PENDING; // Fallback to safe default | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: services/transactions.ts
Line: 63:83
Comment:
The status mapping has a critical flaw: the `default` case (line 82) returns `tx.status` directly without validation. If the API returns an unexpected status string, it will be assigned to the Transaction object even though it doesn't match any `TransactionStatus` enum value. This breaks type safety and could cause runtime errors in components that switch on transaction status.
For example, if the API returns "Processing" or "Initiated", these would be passed through unchanged and not handled properly by the UI.
```suggestion
// Map status from API to TransactionStatus enum
let status: TransactionStatus;
switch (tx.status) {
case "ClaimReady":
status = TransactionStatus.READY_TO_CLAIM;
break;
case "Bridged":
status = TransactionStatus.CLAIMED;
break;
case "Pending":
status = TransactionStatus.PENDING;
break;
default:
Logger.error(`Unknown transaction status from API: ${tx.status}`);
status = TransactionStatus.PENDING; // Fallback to safe default
}
```
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (2)
This polling logic should be removed from Container since the new architecture handles polling in the store/hook layer. The initial fetch can be kept if needed, but the interval should be removed: Prompt To Fix With AIThis is a comment left during a code review.
Path: components/common/container.tsx
Line: 24:54
Comment:
This component creates its own polling interval (15 seconds) while `useTransactions` hook also starts a separate polling interval (20 seconds via `startPolling`). This results in duplicate API calls - transactions will be fetched every 15 seconds from Container AND every 20 seconds from the useTransactions hook.
This polling logic should be removed from Container since the new architecture handles polling in the store/hook layer. The initial fetch can be kept if needed, but the interval should be removed:
```suggestion
useEffect(() => {
if (!address && !selected?.address) {
return;
}
// Only do initial fetch, polling is handled by useTransactions hook
const timeoutId = setTimeout(async () => {
await fetchAllTransactions({
ethAddress: address,
availAddress: selected?.address,
setTransactionLoader,
isInitialFetch: true,
});
}, 500);
return () => {
if (timeoutId) clearTimeout(timeoutId);
};
}, [selected?.address, address, fetchAllTransactions, setTransactionLoader]);
```
How can I resolve this? If you propose a fix, please make it concise.
This inconsistency will cause issues in code that expects sourceTimestamp to be a string. For example, in hooks/useTransactions.ts lines 61-62, the code does Or update the Transaction type definition to use Prompt To Fix With AIThis is a comment left during a code review.
Path: services/bridgeapi.ts
Line: 213:213
Comment:
Type mismatch: `sourceTimestamp` is defined as `string` in types/transaction.ts (line 92), but here you're setting it to `new Date(tx.created_at).getTime()` which returns a `number`.
This inconsistency will cause issues in code that expects sourceTimestamp to be a string. For example, in hooks/useTransactions.ts lines 61-62, the code does `new Date(b.sourceTimestamp).getTime()` which works with both string and number, but it's still a type violation.
```suggestion
sourceTimestamp: new Date(tx.created_at).toISOString(),
```
Or update the Transaction type definition to use `number` instead of `string` for sourceTimestamp.
How can I resolve this? If you propose a fix, please make it concise. |
No description provided.