Skip to content
This repository was archived by the owner on Nov 10, 2023. It is now read-only.

fix: bytes params are truncated #3600

Merged
DiogoSoaress merged 3 commits into from
Mar 9, 2022
Merged

fix: bytes params are truncated #3600

DiogoSoaress merged 3 commits into from
Mar 9, 2022

Conversation

@DiogoSoaress
Copy link
Contributor

@DiogoSoaress DiogoSoaress commented Mar 1, 2022

What it solves

Resolves #3592

How this PR fixes it

Truncates Smart Contracts methods bytes values

How to test it

Open a transaction details, for instance:
eth:0x84443F61efc60D10DA9F9a2398980CD5748394BB/transactions/0xb176b051059e2fea8418e4206cca7e7e4cb99b52c18031a27660914d39991592

  • Observe that params of type bytes are now truncated

Screenshots

Screen Shot 2022-03-01 at 00 18 40

@github-actions
Copy link

github-actions bot commented Mar 1, 2022

CLA Assistant Lite All Contributors have signed the CLA.

@github-actions
Copy link

github-actions bot commented Mar 1, 2022 •
edited
Loading

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total
Report generated by eslint-plus-action

@github-actions
Copy link

github-actions bot commented Mar 1, 2022

Deployment links

🟠 Rinkeby ⚪ Mainnet 🟣 Polygon 🟡 BSC ⚫ Arbitrum 🟢 Gnosis Chain

@coveralls
Copy link

coveralls commented Mar 1, 2022 •
edited
Loading

Pull Request Test Coverage Report for Build 1946166994

  • 0 of 3 (0.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.01%) to 34.856%
Changes Missing Coverage Covered Lines Changed/Added Lines %
src/routes/safe/components/Transactions/TxList/HexEncodedData.tsx 0 1 0.0%
src/routes/safe/components/Transactions/TxList/MethodDetails.tsx 0 1 0.0%
src/routes/safe/components/Transactions/TxList/TxDataRow.tsx 0 1 0.0%
Totals Coverage Status
Change from base Build 1945904847: -0.01%
Covered Lines: 3364
Relevant Lines: 8719
💛 - Coveralls

@github-actions
Copy link

github-actions bot commented Mar 1, 2022 •
edited
Loading

E2E Tests Failed
Check the results here: https://github.com/gnosis/safe-react-e2e-tests/actions/runs/1946216771

Failed tests:

  • ❌ Add an existing safe Add an existing safe
  • ❌ Read-only transaction creation and review Read-only transaction creation and review
  • ❌ Safe Apps List Safe Apps List

iamacook
iamacook reviewed Mar 1, 2022
View reviewed changes
Copy link
Contributor

@iamacook iamacook left a comment

Choose a reason for hiding this comment

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

Looking good dude!

I'm not too sure about the line breaks in the middle of "Show more" though:

image
image

Do you think it's possible to align them a bit better veritcally too?
image

src/routes/safe/components/Balances/SendModal/screens/ContractInteraction/utils/index.ts Outdated
export const isUint = (type: string): boolean => type.indexOf('uint') === 0
export const isInt = (type: string): boolean => type.indexOf('int') === 0
export const isByte = (type: string): boolean => type.indexOf('byte') === 0
export const isBytes = (type: string): boolean => type.indexOf('bytes') === 0
Copy link
Contributor

@iamacook iamacook Mar 1, 2022

Choose a reason for hiding this comment

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

Is "byte" never returned?

Copy link
Contributor Author

@DiogoSoaress DiogoSoaress Mar 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I was checking Solidity documentation and all the byte arrays types start with bytes....
Curiously also found that

Prior to version 0.8.0, byte used to be an alias for bytes1.

I think we'd be safe this way but will revert the expression as it is not adding much value.

Copy link
Contributor

@iamacook iamacook Mar 1, 2022

Choose a reason for hiding this comment

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

I'd suggest renaming the function back then.

@DiogoSoaress
Copy link
Contributor Author

DiogoSoaress commented Mar 1, 2022

I'm not too sure about the line breaks in the middle of "Show more" though

I've reduce the number of characters being displayed when hexData is collapsed.

Do you think it's possible to align them a bit better vertically too?

If we'd go with a monospaced font, byte arrays would display more elegantly. Otherwise I find this one tricky as it requires calculating if we should truncate the value based on the hexData width and not in the number of characters.

@DiogoSoaress DiogoSoaress force-pushed the truncate-bytes-params branch from 6c3a9f4 to 31e5a38 Compare March 1, 2022 19:12
iamacook
iamacook approved these changes Mar 1, 2022
View reviewed changes
Copy link
Contributor

@iamacook iamacook left a comment

Choose a reason for hiding this comment

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

💪

@francovenica
Copy link
Contributor

francovenica commented Mar 3, 2022

Looks good to me:
https://pr3600--safereact.review-safe.gnosisdev.com/app/rin:0xFfDC1BcdeC18b1196e7FA04246295DE3A17972Ac/transactions/queue

image

@iamacook
Copy link
Contributor

iamacook commented Mar 3, 2022

Do you think we should add a comma, separated by lines between these two arrays:

image

@DiogoSoaress DiogoSoaress force-pushed the truncate-bytes-params branch from aed5200 to 20d6ac3 Compare March 7, 2022 14:58
@DiogoSoaress
Copy link
Contributor Author

DiogoSoaress commented Mar 7, 2022 •
edited
Loading

with 4 levels 💪

Screen Shot 2022-03-07 at 15 57 12

@DiogoSoaress DiogoSoaress requested a review from iamacook March 7, 2022 15:05
@francovenica
Copy link
Contributor

francovenica commented Mar 7, 2022

The bytes data look fine
The " , " for nested arrays are also showing fine:
https://pr3600--safereact.review-safe.gnosisdev.com/app/rin:0xFfDC1BcdeC18b1196e7FA04246295DE3A17972Ac/transactions/0xf511f5d98cc1835674e8f3426039e8f2c9058eb20738c99f1bc84d59ae6f904a

Some snapshots on how it looks:
image
image

@DiogoSoaress DiogoSoaress merged commit 3646f80 into dev Mar 9, 2022
@DiogoSoaress DiogoSoaress deleted the truncate-bytes-params branch March 9, 2022 10:03
@github-actions github-actions bot locked and limited conversation to collaborators Mar 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@iamacook iamacook iamacook approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Style] Unify the way bytes params are represented

5 participants

@DiogoSoaress @coveralls @francovenica @iamacook