Conversation
|
CLA Signature Action: Thank you for your submission, we really appreciate it. We ask that you read and sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just by adding a comment to this pull request with this exact sentence:
By commenting with the above message you are agreeing to the terms of the CLA. Your account will be recorded as agreeing to our CLA so you don't need to sign it again for future contributions to this repository. 0 out of 1 committers have signed the CLA. |
|
I haven't tested yet but im wondering if similar issue can happen for ERC721? |
| draftTransaction.sendAsset.details.standard === TokenStandard.ERC1155 && | ||
| draftTransaction.amount.value === '0x0': | ||
| draftTransaction.amount.error = INSUFFICIENT_TOKENS_ERROR | ||
| break; |
There was a problem hiding this comment.
can you add a unit test or e2e test for this case ?
|
@sahar-fehri I haven't seen this issue with ERC721 yet. Good reminder! Will double-check. |
|
@sahar-fehri I can't send ERC721 - every attempt results in frozen MM (429 errors) before I can actually see the next screen. cryptoKitty.movThe fix is working great! 0NFT.movMay be it's worth asking @hesterbruikman what message we want to display: 'Insufficient tokens' or 'Cannot send negative or zero amount of tokens'. |
5acc32d to
1ccff93
Compare
|
Thanks @sleepytanya ! The 429 you got is curious, i wasnt able to reproduce, and it seems okay for ERC721, the UI will display "balance: 1 token" Screen.Recording.2024-05-02.at.15.30.00.mov |
|
@sahar-fehri 429 Rate-Limiting errors are common for users coming through e.g. Tor or commercial VPN services. As an aside, in general we shouldn't assume availability of these endpoints and fail gracefully when they aren't available. |
9dc873f to
aa048e4
Compare
f012b0f to
f3a1896
Compare
56e7a02 to
5fbe37c
Compare
|
Unrelated to my change, the tests are inconsistent and often fail when an address is selected or inserted as the recipient. Common errors are: |
0deb070 to
2fc386c
Compare
2fc386c to
31e6724
Compare
Restore domain-input Fix test for send error Fix max button test E2Es: Update amount selector, Next to Continue button Remove header for send flow Fix address book and ENS tests Update snap-account-transfers.spec.ts and helpers.js Update change-language.spec.js Fix custom-token-send-transfer.spec.js Fix missing NFT icon Make send-nft.spec.js pass, set send volume to 1 when NFT Fix send-edit.spec.js Fix address-book.spec.js Release the address book changes
Signed-off-by: Ethan Wessel <ethan.wessel@consensys.net> Co-authored-by: Ethan Wessel <ethan.wessel@consensys.net>
31e6724 to
1777317
Compare
|
Change was cherry picked out. |
fbf3ac0 to
a3fa6a7
Compare
a3fa6a7 to
6b5250e
Compare
georgewrmarshall
left a comment
There was a problem hiding this comment.
Hey @ejwessel, I've started a discussion in slack regarding component library changes https://consensys.slack.com/archives/C0354T27M5M/p1716478907218739
| } from './button-icon.types'; | ||
|
|
||
| const buttonIconSizeToIconSize: Record<ButtonIconSize, IconSize> = { | ||
| [ButtonIconSize.Xs]: IconSize.Xs, |
There was a problem hiding this comment.
Hey @ejwessel, this method for adding a size seems correct, but changes to the component-library require a thorough review to ensure alignment with the Figma and mobile component. It also appears that this update isn't currently being used. Should this be removed?
Notice the Figma component does not have an Xs size
| href={CONSENSYS_TERMS_OF_USE} | ||
| target="_blank" | ||
| className="quote-card__TOS" | ||
| disableUnderline |
There was a problem hiding this comment.
If the underline on hover must be overridden can we do it with a custom className instead of updating the component api itself?
f748aa0 to
8562dbb
Compare
Description
#23801
Related issues
Fixes:
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist