Fix Deposit Details reactivity #45
Conversation
There is an issue with data returned by this hook. It is not reactive, once read it's not updated as the process proceeds. This commit resolves this issue. Moreover requested and finalized transaction hashes are now gathered basing on deposit key, updated by minting events subscription and combined with deposit details in the hook mentioned above. There is an issue with confirmation numbers which should be updated with polling store effect. From the point of this hook it should be read from the store slice and watched by `useEffect` hook so `depositData` can could be updated.
| useEffect(() => { | ||
| setDepositData( | ||
| (prevState) => | ||
| ({ | ||
| ...prevState, | ||
| optimisticMintingRequestedTxHash, | ||
| optimisticMintingFinalizedTxHash, | ||
| } as DepositData) | ||
| ) | ||
| }, [optimisticMintingFinalizedTxHash, optimisticMintingRequestedTxHash]) | ||
|
|
||
| useEffect(() => { | ||
| setDepositData((prevState) => ({ | ||
| ...prevState, | ||
| confirmations: txConfirmations, | ||
| })) | ||
| }, [txConfirmations]) |
There was a problem hiding this comment.
I would prefer to not put it here and update like that, because it might lead to unexpected behaviour. For example we can have 5/6 confirmations for the given deposit. Let's say we go to the deposit details page and the fetch starts. We are at this line:
const confirmations =
(await threshold.tbtc.getTransactionConfirmations(btcTxHash)) ??
txConfirmations
and confirmations is equal to 5. After that (but before fetch is over) the 6th confirmation happens so we update the confirmations in the state with the value of 6. Then, at the end of the fetch, we overwrite the value with 5 which is out-dated by now.
Same can happen with tx hashes.
The safer way would be to move those two calls:
const { optimisticMintingFinalizedTxHash, optimisticMintingRequestedTxHash } =
useSubscribeToOptimisticMintingEvents(depositKey)
const { txConfirmations } = useTbtcState()
to DepositDetails.tsx and the use double question mark to choose the right value. The values from the state should have a priority.
There was a problem hiding this comment.
As long as the value is updated, the change is captured by useEffect hook and the depositData state is updated accordingly. Notice I'm aggregating the update upon the most recent state value.
Actually the isFetching flag is there only to provide feedback for loading state, nothing more.
Spreading correlated state update across multiple files/hooks is confusing, difficult to maintain and should be avoided. That's why I'm trying to keep everything in one place. If you call useFetchDepositData you expect to get complete deposit data. How could I know to get confirmations count from another place and transaction hashes from one another place? It's overcomplicated - especially if the shape of data provided by the useFetchDepositData hook indicates both confirmations count and hashes are there and (intuitively) should be there up to date but it isn't.
If you disagree and you have idea how to resolve it, please feel free to commit your changes. I ain't got a clue what to do with it 😆
The current solution might lead to unexpected behaviors, because we reset the
deposit details step based on a lot of dependencies when we should clean it
after `depositKey` is changed.
This is a quick summary of how clean-up function behaves based on dependency
array:
1. No Dependency Array: Cleanup runs after every render, before the next effect
or on unmount, ensuring effects are properly cleaned up in a constantly updating
component.
2. Empty Dependency Array (`[]`): Cleanup runs once, when the component
unmounts, mirroring a setup that only needs to run and clean up a single time
(like componentDidMount and componentWillUnmount).
3. With Dependencies (`[deps]`): Cleanup runs when any dependency changes, prior
to the next effect to handle updated dependencies, and on unmount, allowing for
targeted effect management and resource cleanup based on specific state or prop
changes.
This means that in our current case, the cleanup function will fire again when
(for example) the new confirmation for the bitcoin transaction arrives, thus set
the `depositDetailsStep` to `bitcoin-confirmations` . It will work anyway,
because we have another `useEffect` below with `confirmations` in dependency
array that will set the proper step, but I think I came up with better solution
that would be less bug-prone. I've separated the cleanup function to another
useEffect with only `depositKey` as a dependency. I've also set it to
`undefined` instead of `bitcoin-transactions`.
```
useEffect(() => {
return () => {
updateState("depositDetailsStep", undefined)
}
}, [depositKey])
```
While refactoring Deposit Details page for Bitcoin on Base we encountered a problem with data fetch in our new Page Layout. The Page Layout uses the data in two sections which indicated that we might need to move the data fetch to redux-toolkit and store it in the redux store. That's what I did here. The problematic parts before this commit were: 1) `useFetchDepositDetails` hook was used two times - one for main page and one for right section. This is not good because it is a costly operation, 2) No one source of truth - we had two subscriptions that listened for optimistic minting events and we store them both in redux store AND in local state which lead to confusion, 3) Actual implementation of `useFetchDepositDetails` had bugs - if the optimistic minting event was fired during the data fetch, we could potentially override the new data with old data that was fetched during the fetch. To solve this problem I moved everything related to DepositDetails completely to redux store, under tbtcSlice -> depositDetails. The main changes are: 1. Remove unnecessary optimistic minting event subscriptions Up to this point we had two subscriptions for optimistic minting events. The ones placed in `useSubscribeToOptimisticMintingRequestedEvents` and `useSubscribeToOptimisticMintingFinalizedEvents` were subscribing to the event for currently connected account. The other two, placed in `useSubscribeToOptimisticMintingEvents`, were subscribing to the event based on deposit key. Turned out that the `useSubscribeToOptimisticMintingRequestedEvents` is not needed at all and `useSubscribeToOptimisticMintingFinalizedEvents` is only needed to update the bridge activity (which is not currently visible in bitcoin on base). I've decided to move subscriptions from `useSubscribeToOptimisticMintingEvents` to those two file I mentioned so that now they are subscribe to the events based on deposit key. I've also separated subscription to the optimistic minting finalized event (previously `useSubscribeToOptimisticMintingFinalizedEvents`) to a separate hook called `useSubscribeToOptimisticMintingFinalizedEventForCurrentAccount` 2. Move deposit details data to the store This was needed because we needed that data in two places in our layout. I've moved the fetching logic to a separate redux-toolkit effect that is called in deposit details page each time the depositKey parameter is changed. To prevent fetching the data for the wrong deposit, we also keep depositKey in the store, so that the store is not updated if the depositKey in the store was changed. We use useEffect's cleanup function to clearTheDepositData (including depositKey) from the store. 3. Remove unnecessary variables from tbtcSlice We are removing `txConfirmations`, `optimisticMintingRequestedTxHash` and `optimisticMintingFinalizedTxHash` from the tbtcSlice. Well, technically, we are not removing them but moving it to `depositDetails` object in tbtcSlice. 4. Fire confirmation listener after deposit details data is fetched in redux-toolkit Previously we've fired it in useEffect in DepositDetails manually. Now, we are moving it to redux-toolkit and fire after the depositDetails data is fetched. Of course only if it's needed - if the confirmations surpass the required confirmations value then listener is not needed.
As of now, we don't need it in Bitcoin on Base. We don't have a success page for every step like in threshold dashboard.
There was a problem hiding this comment.
LGTM
Would appreciate a quick look at #45 (comment) @kpyszkowski 😉
EDIT:
I will merge it. If we find any errors in the meantime, we will address them in separate PRs
There is an issue with data returned by the
useFetchDepositDetailshook. It is not reactive, once read it's not updated as the process proceeds. This PR resolves this issue.