fix: Show raw data if decodedData doesn't contain any parameters [SW-435] #4622
Conversation
Branch preview✅ Deploy successful! Storybook: |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| expect(getByText('Data (hex-encoded)')).toBeInTheDocument() | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Consider organizing your mock data and expected results in their own files or constants to enhance readability and maintainability. Additionally, verify if the imported path is correct or if there's a need for dynamic imports to optimize performance.
| @@ -49,7 +47,7 @@ export const DecodedData = ({ txData, toInfo }: Props): ReactElement | null => { | |||
| ? 'this Safe Account' | |||
| : addressInfo?.name || toInfo?.name || txData.to.name | |||
| const avatar = addressInfo?.logoUri || toInfo?.logoUri || txData.to.logoUri | |||
| const isFallback = !method && !txData?.dataDecoded?.parameters | |||
| const isFallback = !method || txData?.dataDecoded?.parameters?.length === 0 | |||
|
|
|||
| let decodedData = <></> | |||
| if (txData.dataDecoded && !isFallback) { | |||
There was a problem hiding this comment.
- Simplify the conditional structure in the blocks handling
txDataandtoInfo. Consider reorganizing nestedifstatements for readability. - For
isFallbackcalculation:methodcheck should use logical||, but simplifying direct logic expressions might improve readability.
| @@ -38,8 +38,6 @@ export const DecodedData = ({ txData, toInfo }: Props): ReactElement | null => { | |||
| ) | |||
| } | |||
|
|
|||
| if (!txData) return null | |||
There was a problem hiding this comment.
While writing tests I realised the only reason this condition is here is to satisfy Typescript so I refactored it. Now it will show that txData is defined further down.
| @@ -49,7 +47,7 @@ export const DecodedData = ({ txData, toInfo }: Props): ReactElement | null => { | |||
| ? 'this Safe Account' | |||
| : addressInfo?.name || toInfo?.name || txData.to.name | |||
| const avatar = addressInfo?.logoUri || toInfo?.logoUri || txData.to.logoUri | |||
| const isFallback = !method && !txData?.dataDecoded?.parameters | |||
| const isFallback = !method || txData?.dataDecoded?.parameters?.length === 0 | |||
There was a problem hiding this comment.
This will cover the case listed where there is a method but no parameters. I am not sure if we have the case where there is no method but parameters (don't think so) but in that case it would also fallback to raw data now.
There was a problem hiding this comment.
There are plenty of methods with no parameters.
E.g. WETH wrap/unwrap (value is passed as value, not in parameters), NFT mints etc.
There was a problem hiding this comment.
That case is fine and should now display the raw data instead of "No parameters"
I was talking about the opposite case where potential transactions don't have a method name but parameters for some reason but feels like if the method name is missing then surely the ABI will be missing so there won't be decoded parameters either.
There was a problem hiding this comment.
I wouldn't replace "No parameters" with a raw data string. The raw data could be added to the list below.
There was a problem hiding this comment.
Ah I undertand what you mean now. There are legitimate transactions without parameters so we should clearly tell the user in those cases that there are no parameters but still have the fallback to raw data. Lets display both to them.
There was a problem hiding this comment.
Yes, exactly. And I would move it down, below the separator. It's a bit weird that the copy button would be in front though.
Also maybe we should just always show it, not just when there are no params.
There was a problem hiding this comment.
The part below the separator is the Summary which we don't render for Multisends but we need this raw data especially for the Multisend transactions. I checked if we can just add another divider between but imo it looks strange:
For top-level transactions we always render the raw data under Advanced details.
I would keep it as is since it has a very limited scope (only rendered when we also render "No parameters")
📦 Next.js Bundle Analysis for safe-wallet-webThis analysis was generated by the Next.js Bundle Analysis action. 🤖
|
| Page | Size (compressed) |
|---|---|
global |
1 MB (🟡 +13 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!
Coverage report
Show files with reduced coverage 🔻
Test suite run success1667 tests passing in 227 suites. Report generated by 🧪jest coverage report action from 9d51ce8 |
usame-algan
force-pushed
the
raw-data-fallback
branch
from
3b93dc5 to
c300545
Compare
|
|
||
| expect(getByText('Data (hex-encoded)')).toBeInTheDocument() | ||
| }) | ||
| }) |
There was a problem hiding this comment.
- Consider parameterizing repeated elements in your test cases. Use a test matrix or utility functions to reduce duplication, especially for common inputs and expectations between tests.
- Address DRY violations in mock setup for test consistency and to ease future changes.
| @@ -49,7 +47,7 @@ export const DecodedData = ({ txData, toInfo }: Props): ReactElement | null => { | |||
| ? 'this Safe Account' | |||
| : addressInfo?.name || toInfo?.name || txData.to.name | |||
| const avatar = addressInfo?.logoUri || toInfo?.logoUri || txData.to.logoUri | |||
| const isFallback = !method && !txData?.dataDecoded?.parameters | |||
| const isFallback = !method || txData?.dataDecoded?.parameters?.length === 0 | |||
|
|
|||
| let decodedData = <></> | |||
| if (txData.dataDecoded && !isFallback) { | |||
There was a problem hiding this comment.
Refactor the conditional logic for returning null by consolidating multiple checks into one if statement. Replace the existing redundant conditionals with if (!txData && !toInfo) return null;. This reduces clutter and improves readability.
There was a problem hiding this comment.
Typescript can't deal with that for some reason.
if (!txData && !toInfo) return null
if (!txData && toInfo) return <XYZ />
// txData should be defined from here on out but isn't
| No parameters | ||
| </Typography> | ||
| {hexData && <HexEncodedData title="Data (hex-encoded)" hexData={hexData} />} | ||
| </> | ||
| ) | ||
| } | ||
|
|
There was a problem hiding this comment.
Adding hexData prop requires handling undefined values when it's not provided. Consider using hexData?.length > 0 instead of just hexData for better clarity. The conditional rendering implies hexData should be checked for both existence and non-emptiness.
| }, | ||
| value: '0', | ||
| operation: 0, | ||
| dataDecoded: { |
There was a problem hiding this comment.
- The
toInfoobject is missing thenameproperty in multiple test instances. Confirm if this change is intended; otherwise, inconsistency might affect related logic or conditional rendering, especially if other parts of the code rely on this property.
| @@ -38,8 +38,6 @@ export const DecodedData = ({ txData, toInfo }: Props): ReactElement | null => { | |||
| ) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
The removal of the isFallback check leads to MethodDetails rendering even if there are no parameters which might not be intended. Ensure method presence or conditions are correctly handled to avoid unintended render logic changes.
usame-algan
force-pushed
the
raw-data-fallback
branch
from
194b93a to
26dd98d
Compare
| No parameters | ||
| </Typography> | ||
| {hexData && <HexEncodedData title="Data (hex-encoded)" hexData={hexData} />} | ||
| </> | ||
| ) | ||
| } | ||
|
|
There was a problem hiding this comment.
Review:
- Improve Conditional Rendering:
Current:Suggestion: Utilize optional chaining consistently and prefer nullish coalescing for clarity:if (!data.parameters?.length) {...}
if (!(data.parameters ?? []).length) {...}
| const mockTxData = { | ||
| to: { | ||
| value: '0x874E2190e6B10f5173F00E27E6D5D9F90b7664C4', | ||
| }, |
There was a problem hiding this comment.
- The test description for "shows an Interact with block if there is no txData but toInfo" needs adjustment. The removal of
namefromtoInfoimplies a change in test focus without updating the description. - Consider adding a beforeEach section to minimize duplications when initializing
mockTxData. It reduces redundancy and applies DRY principles. - The test for "only shows Hex encoded data if no decodedData exists" might imply a redundancy; ensure the condition doesn't overlap with other tests.
- Check if
getByTextandqueryByTextcan be consolidated for clarity when both are used in a single case.
| @@ -38,8 +38,6 @@ export const DecodedData = ({ txData, toInfo }: Props): ReactElement | null => { | |||
| ) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Remove isFallback if it's no longer used. Unused variables can clutter the code and confuse future maintainers. Consider refactoring to ensure the logical flow remains clear and concise without introducing redundant checks or variables.
src/components/transactions/TxDetails/TxData/DecodedData/MethodDetails/index.tsx
Show resolved
Hide resolved
…dDetails/index.tsx Co-authored-by: katspaugh <[email protected]>
| @@ -7,21 +8,26 @@ import { Value } from '@/components/transactions/TxDetails/TxData/DecodedData/Va | |||
|
|
|||
| type MethodDetailsProps = { | |||
There was a problem hiding this comment.
Remove the unnecessary extra blank line after the Typography component to maintain code consistency and readability.
|
Lgtm: Before after comparison:
|
What it solves
Resolves SW-435
How this PR fixes it
isFallbackcondition and instead showsHexEncodedDataadditionally when there are no parametersHow to test it
Screenshots
Checklist