-
-
Notifications
You must be signed in to change notification settings - Fork 650
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
Fix RSK EIP1191 checksum issues #4151
base: master
Are you sure you want to change the base?
Conversation
@@ -154,7 +155,7 @@ const FormSchema = object().shape({ | |||
address: object({ | |||
value: string().test( | |||
'check-eth-address', | |||
translateRaw('TO_FIELD_ERROR'), | |||
translateRaw('TO_FIELD_ERROR', { $network: DEFAULT_NETWORK }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I provided network
to avoid it to be displayed as Enter a valid $network address, ENS name, or blockchain domain
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't you move the schema inside the component so it can access the network
prop and use that instead?
/ok-to-test |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4151 +/- ##
==========================================
- Coverage 77.43% 77.42% -0.02%
==========================================
Files 551 551
Lines 12016 12018 +2
Branches 3197 3197
==========================================
Hits 9305 9305
- Misses 2678 2680 +2
Partials 33 33 ☔ View full report in Codecov by Sentry. |
[sc-7907] |
This pull request has been linked to Shortcut Story #7907: Fix address validation for RSK network in custom token form. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good - a few small comments!
address: string, | ||
resolutionErr: ResolutionError | undefined | ||
resolutionErr: ResolutionError | undefined, | ||
network: Network = NETWORKS_CONFIG.Ethereum |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would prefer having network
be undefined by default here. You can use ?? DEFAULT_NETWORK
in cases where it is undefined. NETWORK_CONFIG
is not the correct type.
@@ -154,7 +155,7 @@ const FormSchema = object().shape({ | |||
address: object({ | |||
value: string().test( | |||
'check-eth-address', | |||
translateRaw('TO_FIELD_ERROR'), | |||
translateRaw('TO_FIELD_ERROR', { $network: DEFAULT_NETWORK }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't you move the schema inside the component so it can access the network
prop and use that instead?
schema.test('valid', translateRaw('TO_FIELD_ERROR', { $network: network.name }), function ( | ||
value | ||
) { | ||
return value && value.value && isValidAddress(value.value, network.chainId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks to work, but I am not sure it will be possible to send without fixing the issues in Ethers. Balance scanning etc seems to be failing on RSK right now. Have you talked to ricmoo or is there an open issue?
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Don't close. @7alip have you looked more at this? |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Description
I created new PR according to requesting changes in #4139
Changes
isValidETHRecipientAddress
withisValidRecipientAddress
which now takes chainId parameter.network
parameter toTO_FIELD_ERROR
translation.providerHandler
to find a better solution.This changes are supposed to fix checksum errors in the following scenarios:
Issues
There are more scenarios where EIP1191 checksum validation needs to be fixed.
We discussed for the better solution as to have support for it in Ethers.js.