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.

Sign up for GitHub

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

Jump to bottom

feat(suite): display contract address error #14220

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from
Draft

feat(suite): display contract address error #14220

wants to merge 1 commit into from
+137 −72

Conversation

enjojoy
Copy link
Contributor

@enjojoy enjojoy commented Sep 9, 2024 •
edited
Loading

Description

If the user enters a contract address, he can proceed with sending only after pressing the "I understand the risk" button

Related Issue

Resolve #7244

Screenshots:

image

@enjojoy enjojoy force-pushed the feat/warn-sending-eth-to-contract branch 3 times, most recently from bf6df91 to f61780b Compare September 10, 2024 11:59
@enjojoy enjojoy force-pushed the feat/warn-sending-eth-to-contract branch from f61780b to f5cc569 Compare September 10, 2024 13:26
@MiroslavProchazka
Copy link
Contributor

MiroslavProchazka commented Sep 10, 2024 •
edited
Loading

Nit - copy adjustment:

"“This is a contract address. Are you sure you want to proceed with sending to this address?”"

wdyt @Hermez-cz ?

@enjojoy
Copy link
Contributor Author

enjojoy commented Sep 10, 2024

Nit - copy adjustment:

"“This is a contract address. Are you sure you want to proceed with sending to this address?”"

wdyt @Hermez-cz ?

I discussed it with Ben and he also proposed the "This". Also I will add a Learn More button with a link to this article https://trezor.io/support/a/where-is-my-ethereum

@enjojoy enjojoy force-pushed the feat/warn-sending-eth-to-contract branch from f5cc569 to 386bbcd Compare September 10, 2024 16:58
@enjojoy enjojoy force-pushed the feat/warn-sending-eth-to-contract branch 2 times, most recently from 257d292 to a65ee1d Compare September 10, 2024 17:02
@enjojoy
Copy link
Contributor Author

enjojoy commented Sep 10, 2024

@MiroslavProchazka I updated the screenshot

@enjojoy enjojoy force-pushed the feat/warn-sending-eth-to-contract branch from a65ee1d to 4e110f6 Compare September 11, 2024 09:18
@tomasklim
Copy link
Member

Please consider smart contract wallets (Account Abstraction). e.g. https://safe.global/wallet
More and more often people will intentionally want to send it there. Probably just inform Ben about it (maybe the warning should not be that strong) and update KB https://www.halborn.com/blog/post/what-is-a-smart-contract-wallet

tomasklim
tomasklim requested changes Sep 12, 2024
View reviewed changes
packages/suite/src/components/wallet/InputError.tsx Outdated Show resolved Hide resolved
packages/suite/src/support/messages.ts Outdated Show resolved Hide resolved
packages/suite/src/views/wallet/send/Outputs/Address.tsx Outdated Show resolved Hide resolved
packages/suite/src/views/wallet/send/Outputs/Address.tsx Outdated
return {
onClick: () => {
contractAddressWarningDismissed = true;
trigger(inputName);
Copy link
Member

Choose a reason for hiding this comment

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

what does this do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

trigger starts re-validation. same as setValue, but without changing the value.

https://react-hook-form.com/docs/useform/trigger

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's needed to remove the contract warning

packages/suite/src/views/wallet/send/Outputs/Address.tsx Outdated Show resolved Hide resolved
packages/suite/src/views/wallet/send/Outputs/Address.tsx Outdated Show resolved Hide resolved
@enjojoy enjojoy force-pushed the feat/warn-sending-eth-to-contract branch 6 times, most recently from d93700f to e64fd5b Compare September 14, 2024 16:19
@enjojoy enjojoy force-pushed the feat/warn-sending-eth-to-contract branch 2 times, most recently from 2066b41 to cbf0471 Compare September 16, 2024 09:14
@enjojoy enjojoy force-pushed the feat/warn-sending-eth-to-contract branch from 4aa4a18 to fc826a0 Compare September 17, 2024 10:53
@enjojoy
Copy link
Contributor Author

enjojoy commented Sep 17, 2024

I updated the screenshot with a new version of the shorter and less implicit version after discussing it with Ben and reminded him about the KB article update @tomasklim

@enjojoy enjojoy force-pushed the feat/warn-sending-eth-to-contract branch from ecce9ea to d977b75 Compare September 17, 2024 13:38
@jvaclavik
Copy link
Contributor

Btw I think "I understand the risk" shouldn't be a button. Because button evokes you might be redirected somewhere else (different page, modal or something else).
This is pretty clear use case for checkbox and it shouldn't be possible to send form without ticking it.

@Hermez-cz WDYT?

@enjojoy enjojoy force-pushed the feat/warn-sending-eth-to-contract branch 2 times, most recently from 6a17127 to 93b8999 Compare September 19, 2024 13:01
@enjojoy enjojoy self-assigned this Sep 19, 2024
@Hermez-cz
Copy link

Btw I think "I understand the risk" shouldn't be a button. Because button evokes you might be redirected somewhere else (different page, modal or something else). This is pretty clear use case for checkbox and it shouldn't be possible to send form without ticking it.

@Hermez-cz WDYT?

Hi, the design linked in #7244 is already quite old, but the button would make sense in there as part of the warning banner to dismiss it. In the current implementation of an error message, the button really feels odd. Checkbox makes better logical sense, but might make the interface fuzzy and the flow not so clear. Will have to check with the designer.

@enjojoy enjojoy force-pushed the feat/warn-sending-eth-to-contract branch from 93b8999 to 79d506c Compare September 20, 2024 11:17
@tomasklim
Copy link
Member

@enjojoy 🚨🚨📢 does it work for Polygon and BNB?

@enjojoy enjojoy force-pushed the feat/warn-sending-eth-to-contract branch from 79d506c to 3c6472e Compare September 25, 2024 16:42
@enjojoy
Copy link
Contributor Author

enjojoy commented Sep 25, 2024

@enjojoy 🚨🚨📢 does it work for Polygon and BNB?

It works on Polygon but not on BNB, because BNB smart contracts don't provide any contract info for some reason, it's the only way I can know if it's a smart contract address

@enjojoy
Copy link
Contributor Author

enjojoy commented Sep 30, 2024

❌ Blocked - no contractInfo on BNB

@enjojoy enjojoy added the blocked Blocked by external force. Third party inputs required. label Sep 30, 2024
@tomasklim tomasklim marked this pull request as draft October 26, 2024 10:29
@tomasklim
Copy link
Member

tomasklim commented Oct 26, 2024 •
edited
Loading

Maybe we can use rpc calls to node using logic added here https://github.com/trezor/trezor-suite/pull/12122/commits we cant, we have eth_call not eth_getAccount

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MiroslavProchazka MiroslavProchazka Awaiting requested review from MiroslavProchazka MiroslavProchazka is a code owner

@jvaclavik jvaclavik Awaiting requested review from jvaclavik jvaclavik is a code owner

@marekrjpolak marekrjpolak Awaiting requested review from marekrjpolak marekrjpolak is a code owner

@tomasklim tomasklim Awaiting requested review from tomasklim

@adamhavel adamhavel Awaiting requested review from adamhavel adamhavel is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

@enjojoy enjojoy

Labels
blocked Blocked by external force. Third party inputs required.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Warn about sending funds to a contract address
5 participants
@enjojoy @MiroslavProchazka @tomasklim @jvaclavik @Hermez-cz