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

remove code duplication by abstracting tx selection and tx status changes events #7034

Closed
NeOMakinG opened this issue May 31, 2024 · 3 comments · Fixed by #7043
Closed

remove code duplication by abstracting tx selection and tx status changes events #7034

NeOMakinG opened this issue May 31, 2024 · 3 comments · Fixed by #7043

Comments

@NeOMakinG
Copy link
Collaborator

NeOMakinG commented May 31, 2024

Overview

I noticed reviewing RFox things that we often duplicate a few TX status logic, there are duplicated useEffects managing the tx status changes event that could be abstracted into a single hook. This could lead to errors if the effect is wrongly implemented at one place. We are able to avoid the risks by implementing an abstraction.

We have to discuss the final implementation but I feel like we could implement a useTx hook that could also implement a few callbacks:

type UseTxStatusProps = {
  accountId: AccountId
  txId: string
  onTxStatusConfirmed?: () => Promise<void>
  onTxStatusPending?: () => Promise<void>
  onTxStatusFailed?: () => Promise<void>
  onTxStatusChanged?: (status: TxStatus) => Promise<void>
}

References and additional details

It create a lot of code duplicates :

(

const stakingAssetAccountAddress = useMemo(
() => fromAccountId(confirmedQuote.stakingAssetAccountId).account,
[confirmedQuote.stakingAssetAccountId],
)
const stakingAsset = useAppSelector(state =>
selectAssetById(state, confirmedQuote.stakingAssetId),
)
const serializedTxIndex = useMemo(() => {
return serializeTxIndex(confirmedQuote.stakingAssetAccountId, txId, stakingAssetAccountAddress)
}, [confirmedQuote.stakingAssetAccountId, stakingAssetAccountAddress, txId])
const txLink = useMemo(
() => getTxLink({ txId, defaultExplorerBaseUrl: stakingAsset?.explorerTxLink ?? '' }),
[stakingAsset?.explorerTxLink, txId],
)
const tx = useAppSelector(state => selectTxById(state, serializedTxIndex))
useEffect(() => {
if (tx?.status !== TxStatus.Confirmed) return
handleTxConfirmed()
}, [handleTxConfirmed, tx?.status])
)

Acceptance Criteria

  • The useTxStatus abstraction has been implemented
  • every useEffects running on TxStatus are replaced by the callbacks of this new abstraction
  • every tx selection logic have been replaced by this new abstraction

Need By Date

No response

Screenshots/Mockups

No response

Estimated effort

No response

@NeOMakinG
Copy link
Collaborator Author

Here is the POC: #7043

@0xean
Copy link
Contributor

0xean commented Jun 3, 2024

To be addressed post rFOX launch

@0xean
Copy link
Contributor

0xean commented Jun 24, 2024

  • where can we use this (outside of rfox also)
  • regression testing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants