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

Strip network prefix from the Address when calling EthHashInfo #3037

Merged

Conversation

DiogoSoaress
Copy link
Member

@DiogoSoaress DiogoSoaress commented Nov 22, 2021

What it solves

#3015 (comment) (Bug when uploading a QR code containing a chain prefix in the "Send Collectibles")
Resolves #3036

How this PR fixes it

  • Pass the Address without prefix to EthHashInfo and let the appearance settings decide if prefix should be displayed
  • Removes the chain prefix from "Transaction hash" and "SafeTxHash" fields in the Tx List

How to test it

From #3015 (comment)
Screen Shot 2021-11-22 at 16 49 42

Screenshots

Screen Shot 2021-11-22 at 16 52 39

@github-actions
Copy link

CLA Assistant Lite All Contributors have signed the CLA.

@github-actions
Copy link

github-actions bot commented Nov 22, 2021

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
Copy link

github-actions bot commented Nov 22, 2021

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

Failed tests:

  • ❌ Add an existing safe Add an existing safe
  • ❌ Address book Address book
  • ❌ Safe Apps List Safe Apps List
  • ❌ Read-only transaction creation and review Create and review a Send Funds transaction

Copy link
Member

@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.

It's a bit convoluted not using the props as they are intended.

@@ -12,7 +13,7 @@ const PrefixedEthHashInfo = ({ hash, ...rest }: Props): ReactElement => {

return (
<EthHashInfo
hash={hash}
hash={getAddressWithoutNetworkPrefix(hash)}
Copy link
Member

Choose a reason for hiding this comment

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

Why are you extracting it here and not sending them via their respective props?

Copy link
Member Author

Choose a reason for hiding this comment

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

I opted to implement it in the wrapper as a input sanitization. I don't think it invalidates the correct use of the hash prop.

Doing this in the wrapper is also a way to address it in one single place and to not repeat it every time we call the <PrefixedEthHashInfo /> component.

Let me know if it makes sense. It is a simple tweak otherwise.

Copy link
Member

@iamacook iamacook Nov 23, 2021

Choose a reason for hiding this comment

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

I think it makes the most sense to do this within the component. If you are intent on doing it in the wrapper, I think it's cleaner and more understandable to parse the hash directly and output the props accordingly, i.e.

  const getPrefixedHashInfoProps = (hash: string, shouldShowShortName: boolean ): { shortName: string | undefined, hash: string }

  <EthHashInfo
    {...getPrefixedHashInfoProps(hash, shouldShowShortName)}
    shouldShowShortName={showChainPrefix}
    shouldCopyShortName={copyChainPrefix}
  />

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure how this improves the solution. My view is that shortName should be calculated independently of the hash and vice versa.

The way to go would be maybe update the <EthHashInfo /> to have the input sanitization as the component responsability.

Copy link
Member Author

Choose a reason for hiding this comment

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

@iamacook created a issue to tackle it 5afe/safe-react-components#171

@DiogoSoaress DiogoSoaress merged commit 6809c9d into feature/eip-3770-prefixes Nov 23, 2021
@DiogoSoaress DiogoSoaress deleted the pass-prefixless-address-to-ethhashinfo branch November 23, 2021 15:29
@github-actions github-actions bot locked and limited conversation to collaborators Nov 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EIP-3770: Don't use prefix for tx and safe hash
2 participants