-
Notifications
You must be signed in to change notification settings - Fork 634
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
[CHORE]: Remove transaction inconsistencies and improve type safety #6137
Conversation
src/components/ActivityIndicator.tsx
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typescript conversion
src/__swaps__/types/refraction.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved all txn types into @/entities
package.json
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this types declaration file was missing when converting ActivityIndicator to typescript
src/components/Divider.tsx
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typescript conversion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
=> .tsx
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
const ActivityListHeaderHeight = 42; | ||
const TRANSACTION_COIN_ROW_VERTICAL_PADDING = 7; | ||
|
||
const getItemLayout = sectionListGetItemLayout({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was causing issues with the SectionList below, so I added a @ts-expect-error
declaration for now. Maybe bumping react-native
will fix it? But not worried about it for now.
const keyExtractor = (data: TransactionSections['data'][number]) => { | ||
if ('hash' in data) { | ||
return (data.hash || data.timestamp ? data.timestamp?.toString() : performance.now().toString()) ?? performance.now().toString(); | ||
} | ||
return ( | ||
(data.displayDetails?.timestampInMs ? data.displayDetails.timestampInMs.toString() : performance.now().toString()) ?? | ||
performance.now().toString() | ||
); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need the key extractor to always fallback to a string, so theoretically we should never hit performance.now
calls, but added them in case we do.
const { accountAddress, nativeCurrency } = useAccountSettings(); | ||
|
||
const { setScrollToTopRef } = useSectionListScrollToTopContext(); | ||
const { sections, nextPage, transactionsCount, remainingItemsLabel } = useAccountTransactions(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved the sections and transaction stuff in here instead of in ProfileScreen
since we don't need to re-render the whole page just the list.
|
||
const handleScrollToTopRef = (ref: SectionList<TransactionItemForSectionList, TransactionSections> | null) => { | ||
if (!ref) return; | ||
// @ts-expect-error - no idea why this is not working |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
welcoming ideas on how to get this to work if anyone has one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const { setScrollToTopRef } = useSectionListScrollToTopContext<TransactionItemForSectionList, TransactionSections>();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
THANK YOU
}} | ||
testID={'wallet-activity-list'} | ||
ListEmptyComponent={<ActivityListEmptyState />} | ||
// @ts-expect-error - mismatch between react-native-section-list-get-item-layout and SectionList |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const getStatusTitle = useCallback((status: TransactionStatus, title: string) => { | ||
const transactionType = lang.l.transactions.type[title as keyof typeof lang.l.transactions.type]; | ||
if (typeof transactionType === 'string') { | ||
return transactionType; | ||
} | ||
return transactionType[status as keyof typeof transactionType] || ''; | ||
}, []); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if this was intentional or not, but our i18n for some keys had pending | failed | confirmed
keys inside of lang.l.transactions.type
. I need to test this to confirm.
@@ -61,7 +102,7 @@ export interface RainbowTransaction { | |||
nonce?: number | null; | |||
protocol?: ProtocolType | null; | |||
flashbots?: boolean; | |||
approvalAmount?: 'UNLIMITED' | (string & object); | |||
approvalAmount?: 'UNLIMITED' | (string & Record<string, never>); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keeps type inference but loosens it at the same time
export enum FlashbotsStatus { | ||
PENDING = 'PENDING', | ||
INCLUDED = 'INCLUDED', | ||
FAILED = 'FAILED', | ||
CANCELLED = 'CANCELLED', | ||
UNKNOWN = 'UNKNOWN', | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these were used in a couple different files, so collocated it here and expanded it.
src/handlers/transactions.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lot of stuff in this file was unused so just cleaned up
} | ||
return { | ||
sections: requestsToApprove.concat(sectionedTransactions), | ||
sections: sectionedTransactions as SectionListData<TransactionItemForSectionList, TransactionSections>[], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't love this, but unsure how to make it work without casts
export { getRequestDisplayDetails } from './requests'; | ||
export { getDescription } from './transactions'; | ||
export { getDescription, convertNewTransactionToRainbowTransaction } from './transactions'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed parseNewTransaction
to convertNewTransactionToRainbowTransaction
to properly describe what it does
@@ -55,7 +61,7 @@ export async function claimBridge({ parameters, wallet, baseNonce }: ActionProps | |||
const provider = getProvider({ chainId: ChainId.optimism }); | |||
|
|||
const l1GasFeeOptimism = await ethereumUtils.calculateL1FeeOptimism( | |||
// @ts-ignore | |||
// @ts-expect-error - TODO: fix improper arguments here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will be fixed in the send sheet PR, so just ts-expect-error'd it for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @benisgold for visibility, assuming i will need to rebase once your raps work is in?
import { ActivityList } from '../components/activity-list'; | ||
import { Page } from '../components/layout'; | ||
import { useNavigation } from '../navigation/Navigation'; | ||
import { ButtonPressAnimation } from '@/components/animations'; | ||
import { useAccountProfile, useAccountSettings, useAccountTransactions, useRequests } from '@/hooks'; | ||
import { useAccountProfile, useAccountSettings, useAccountTransactions } from '@/hooks'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useRequests
was unused so cleaned up this file a lot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff. Just one question. ✅
useEffect(() => { | ||
if (isLoading) { | ||
onPress(); | ||
setIsLoading(false); | ||
} | ||
}, [isLoading, setIsLoading, onPress]); | ||
const onPressWrapper = () => { | ||
setIsLoading(true); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how does this work?
onPress sets isLoading
to true
, rerenders run props.onPress
, then right after sets isLoading
to false
wouldn't that just quickly flash the loading component? unless onPress is doing some crazy expensive blocking work
is it not the same as
const onPressWrapper = () => {
setIsLoading(true);
onPress();
setIsLoading(false);
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I didn't originally write this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
afaik this is just for fetching the next batch of transactions
export enum TransactionStatus { | ||
approved = 'approved', | ||
approving = 'approving', | ||
bridging = 'bridging', | ||
bridged = 'bridged', | ||
cancelled = 'cancelled', | ||
cancelling = 'cancelling', | ||
confirmed = 'confirmed', | ||
contract_interaction = 'contract interaction', | ||
deposited = 'deposited', | ||
depositing = 'depositing', | ||
dropped = 'dropped', | ||
failed = 'failed', | ||
minted = 'minted', | ||
minting = 'minting', | ||
pending = 'pending', | ||
purchased = 'purchased', | ||
purchasing = 'purchasing', | ||
received = 'received', | ||
receiving = 'receiving', | ||
self = 'self', | ||
selling = 'selling', | ||
sold = 'sold', | ||
sending = 'sending', | ||
sent = 'sent', | ||
speeding_up = 'speeding up', | ||
swapped = 'swapped', | ||
swapping = 'swapping', | ||
unknown = 'unknown status', | ||
withdrawing = 'withdrawing', | ||
withdrew = 'withdrew', | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's this for?
It was a pain to refactor something similar in the bx, I think it's more manageable to have transaction status as 'confirmed' | 'pending' | 'failed'
and then in conjunction with transaction type 'swap' | 'send' | 'mint' | ...
we derive the label like a type: 'swap', status: 'confirmed'
= 'swapped'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can further be cleaned up, not going to tackle that in this PR as it's already a lot.
What changed (plus any additional context for devs)
This PR cleans up a lot of the unused code stemming back from the transaction redux => Zustand refactor. A lot of our types were duplicated and all over the place. Now they all are in
@/entities/transactions/...
and can be referenced via@/entities
.I also coupled this PR with cleaning up the
ProfileScreen
=>ActivityList
since it's touching 100% of the transactions code. Noticed lots of things once converting those two files to typescript.Note there are still a bit of casts
as
in this PR that I want to create smaller follow-up PRs forScreen recordings / screenshots
Simulator.Screen.Recording.-.iPhone.16.Pro.-.2024-10-01.at.13.54.08.mp4
What to test
All things transactions