2218 BugFix: Queued Transactions not showing up
#2223
Conversation
… data for transaction
✅ Deploy Preview for decent-interface-prod ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for decent-interface-dev ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
@Da-Colon - see test results below Ensure that Multisig queued Proposals show up ✅ Detail --> Ensure that Multisig queued Proposals show up ✅
Detail --> Ensure Transactions (Transfers) show up on Treasury Page (any safe) - ❌ 
|
Yeah works on both the Prod and dev preview links for me? |
|
@Da-Colon look closer at Tom's screenshot for "on app.dev i get this" it includes a transaction at the top for incoming 425 USDC from 4 days ago. that TX is not on this PR's deploy preview (according to Tom) |
- also adds sorting transfers by blockNumber
I see it now. Sorted and Fixed |
| transactionHash: | ||
| multiSigTransaction.transactionHash || | ||
| (transaction as SafeMultisigTransactionWithTransfersResponse).safeTxHash, | ||
| // ! @todo This is why we are showing the two different hashes |
There was a problem hiding this comment.
If I remember this right - we used to be also showing different copies in this case(so instead of Transaction Hash it was something like Safe Tx Hash or something), not sure why that logic was removed. I think we should add this back
There was a problem hiding this comment.
I'm not following? Why do we need the SafeTxHash?
There was a problem hiding this comment.
Cause that's what eventually is shown to the end user, and we're confusing user by showing 2 different hashes before and after execution
We used to show both - now we just mixed them up
There was a problem hiding this comment.
Ah yeah that's why i removed it so that it would only show the transactionHash. In the details screen right after you execute. There would be a clickable transaction hash that shows to the user and takes them to etherscan. But the safeTxHash comes back first so for a while that is what is used in the navigation (which shows nothing on etherscan)
There was a problem hiding this comment.
I guess we can actually link to Safe UI with the SafeTxHash, but maybe that's more of a product/design question.
@nicolaus-sherrill @xraystyle1980 @xanaramoss wdyt?
There was a problem hiding this comment.
The Safe TX Hash is all that we have when a proposal is "pending" (not executed onchain yet). For Azorius this is never a problem because proposals are onchain, but for Multisig proposals are not onchain so we don't have a transaction to link them to.
@Da-Colon does this track?
@mudrila is the behavior you want to see here currently implemented correctly on prod? dev?
There was a problem hiding this comment.
O we are using it somewhere to track the state of things?
There was a problem hiding this comment.
well idk if we're using it to track the state of things. I'm just saying that it's the best identifier we have for multisig transactions which haven't executed onchain yet. and there are places in the UI where @mudrila thinks some behavior or information has possibly unexpectedly changed.
There was a problem hiding this comment.
@adamgall @Da-Colon From what I can tell - we currently aren't using this Safe Tx Hash on dev/prod - which I'm not sure when changed.
I'm just stating that before we used to differentiate on code and UI level between Safe Tx Hash and on chain Tx Hash and showing both of them. As far as I'm remembering this right - we also used to link to Safe UI with that "Safe Tx Hash" - which I believe we should be doing.
My take/suggestion here is:
- Put a ticket to properly process Safe Tx Hash - whether via swapping label/value in the UI or via showing both Tx Hash and Safe Tx Hash when it's applicable. It is certainly p2 and I don't see that reasonable to focus on this discussion right now.
- Gain context from @tomstuart123 and @decentdao/design on how we'll be proceeding forward
- Resolve this conversation and 🚢
💩 this to prod
- update syntax - update native token address to MOCK_MORALIS_ETH_ADDRESS - compare indices instead of object
| transactionHash: | ||
| multiSigTransaction.transactionHash || | ||
| (transaction as SafeMultisigTransactionWithTransfersResponse).safeTxHash, | ||
| // ! @todo This is why we are showing the two different hashes |
There was a problem hiding this comment.
The Safe TX Hash is all that we have when a proposal is "pending" (not executed onchain yet). For Azorius this is never a problem because proposals are onchain, but for Multisig proposals are not onchain so we don't have a transaction to link them to.
@Da-Colon does this track?
@mudrila is the behavior you want to see here currently implemented correctly on prod? dev?
|
@Da-Colon just gave my approval, however i would like to see all open conversations in this PR resolved before you merge. thank you! |
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Fixes #2218
This PR replaces the
getAllTransactionAPI request withgetMultisigTransactionsrequest. Most of the properties are the same except for thetransfersproperty.There are two spots where this is changes somethings.
Dashboard:
Will no longer show Treasury activities. These would be either a Card itself or added text to the bottom of the description that would display any transfer information.
Treasury:
Was able to use a combination of
getIncomingTransactionsandgetTokenfrom Safe'sapi-kitto retrieve the same data and put Treasury back together.Testing