-
Notifications
You must be signed in to change notification settings - Fork 159
fix: display correct cross-chain order summary #6768
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
Conversation
TODO: decompose useRecentActivity.ts file
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (19)
Tip You can disable sequence diagrams in the walkthrough.Disable the ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
elena-zh
left a comment
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.
Hey @shoom3301 , overall, great job! Looks way better for cancellations amounts.
However, I have some questions to clarify:
- AFAIU, 'recipient' will be removed a bit later from all these messages. correct?
- If you so, could you please add a receiver if it was specified for a swap and bridge order? E.g. here I specified a custom recipient
but the modals/pop-up shows me a wrong one.
Or it should be fixed here?
- I'm a bit confused with amounts. I see one 'TO' amount on the progress bar, and another one on the cancelled pop-up. I understand where this amount comes from, however, not sure what is the best option here: leave it as it is or still show an 'initial amount' for bridge, as users placed a bridge order.
I loke more the 2nd option, how is it difficult to implement?
Thank you!
limitofzero
left a comment
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.
…ol/cowswap into refactor/order-summary-1
… refactor/order-summary-1 # Conflicts: # apps/cowswap-frontend/src/common/containers/CancellationModal/index.tsx # apps/cowswap-frontend/src/common/pure/CancellationModal/RequestCancellationModal.tsx # apps/cowswap-frontend/src/common/updaters/orders/utils.ts # apps/cowswap-frontend/src/modules/account/containers/Transaction/ActivityDetails.tsx

Summary
Fixes #6748 (only 1st and 2nd points)
There is a preparation for this PR: #6767
Please, review this PR commit by commit:
RequestCancellationModal. No functional changes, just cleaning of the code. c7962be#diff-3df799ccd7ff9b27678cd609f3a616cec967ff9698a52ad1cf1af309ea9a5e8acomputeOrderSummaryin order to simplify it. Again, no functional changes, just getting rid oflet. cf33725#diff-5315414acf2e0d3d9c748227b2526fe4f99db49ad92cd642241e9f3420320e4fuseGetSerializedBridgeOrder. 1ebc832#diff-09b05a4ab56ccb1f0bc18d6e98e98373c670f37d9c0e7dc485b8dd6630c2ae51computeOrderSummarydoesn't support bridge orders. In this commit I addedbridgeOrderFromStoreandbridgeOrderFromApiproperties. First one is used when bridging order is pending, another one when it's filled. 0c70318#diff-5315414acf2e0d3d9c748227b2526fe4f99db49ad92cd642241e9f3420320e4fcomputeOrderSummarysupports bridging orders, we wire up them with components where order summary is rendered:ActivityDetailsandCancellationModal. 8f4a80eisBridgeOrderwhen order is cancelled, but this is not correct because we useisBridgeOrderinuseSwapAndBridgeContext(). That hook returns an intermediate token of the order, which is needed to render amounts. 9334831To Test
1st and 2nd points of #6748
Summary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.