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

Refactor: rm unnecessary tx decoding #4541

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from
Open

Refactor: rm unnecessary tx decoding #4541

wants to merge 8 commits into from

Conversation

katspaugh
Copy link
Member

@katspaugh katspaugh commented Nov 20, 2024

What it solves

A follow-up on #4177 that partially addresses #4331.

How it solves it

  • Remove unnecessary useDecodeTx calls because we now always propose txs, so they already have txInfo and txData and don't need to be decoded from raw tx data
  • Unify the handling of Swap and Stake transaction confirmations

How to test it

  • Make a swap tx
  • Make a staking tx
  • Make a threshold change tx

All the txs should work exactly as before.

Copy link

github-actions bot commented Nov 20, 2024

Copy link

github-actions bot commented Nov 20, 2024

📦 Next.js Bundle Analysis for safe-wallet-web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

🎉 Global Bundle Size Decreased

Page Size (compressed)
global 1 MB (🟢 -124 B)
Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

Copy link

github-actions bot commented Nov 20, 2024

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
74.24% (-0.13% 🔻)
14015/18877
🔴 Branches
52.3% (-0.42% 🔻)
3458/6612
🔴 Functions
57.89% (-0.33% 🔻)
2047/3536
🟡 Lines
75.88% (-0.11% 🔻)
12730/16776
Show new covered files 🐣
St.
File Statements Branches Functions Lines
🟢
... / index.tsx
66.67% 0% 0% 100%
🟢
... / index.tsx
66.67% 100% 0% 100%
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟡
... / transaction-guards.ts
72.24% (-5.71% 🔻)
40.22% (-21.74% 🔻)
70.59% (-10.29% 🔻)
73.18% (-6.7% 🔻)
🟢
... / index.tsx
80% (-14.29% 🔻)
33.33% (-42.86% 🔻)
50% (-16.67% 🔻)
81.25% (-15.63% 🔻)
🟡
... / index.tsx
54.55% (-45.45% 🔻)
0% (-66.67% 🔻)
0% (-100% 🔻)
60% (-40% 🔻)
🟢
... / index.tsx
96% (-0.15% 🔻)
72.22% (+4.04% 🔼)
100% 100%
🟢
... / tokenTransferParams.ts
96.3% (-3.7% 🔻)
80%
80% (-20% 🔻)
95.83% (-4.17% 🔻)
🟢
... / Deposit.tsx
76.92% (-1.65% 🔻)
0% 0%
83.33% (-1.28% 🔻)

Test suite run success

1618 tests passing in 217 suites.

Report generated by 🧪jest coverage report action from 4ba74ef

@katspaugh katspaugh marked this pull request as draft November 20, 2024 16:03
@katspaugh
Copy link
Member Author

katspaugh commented Nov 20, 2024

Todo: test how pre-proposals work in counterfactual Safes.

Counterfactual Safes cannot create transactions, so all good.

@katspaugh katspaugh marked this pull request as ready for review November 21, 2024 11:20
@katspaugh katspaugh requested review from usame-algan and removed request for clovisdasilvaneto November 21, 2024 11:20
Copy link
Member

@usame-algan usame-algan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a unit-test failing because faker randomly generated an address that contains 420. We might want to choose a number thats more "random" to make that test more robust.

Comment on lines 7 to 14
function SwapOrder({ txDetails, txInfo }: SwapOrderProps) {
return (
<SwapOrderConfirmation
order={txInfo as unknown as AnySwapOrderConfirmationView}
settlementContract={txDetails?.txData?.to?.value ?? ''}
/>
)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've checked the usage of SwapOrder and we only use it in places where we have guards for checking txInfo. Can't we type txInfo here to be more narrow (i.e. Order) instead of still resolving to TransactionInfo? That way we wouldn't need to cast.

This would match the structure of StakingTxProps

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, fixed.

src/components/tx/confirmation-views/index.tsx Outdated Show resolved Hide resolved
katspaugh and others added 3 commits November 22, 2024 10:17
Co-authored-by: Usame Algan <5880855+usame-algan@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Ready for QA
Development

Successfully merging this pull request may close these issues.

2 participants