Fix to infinite loading on approve screen #14756
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
Builds ready [d1f8da5]Page Load Metrics (1631 ± 30 ms)
|
adonesky1
changed the title
adonesky1
changed the title
adonesky1
force-pushed
the
fix-approve-infinite-spin
branch
from
c6ee02e to
99e6fb6
Compare
Builds ready [99e6fb6]Page Load Metrics (1736 ± 51 ms)
|
|
While this is definitely better than infinite loading we should probably do a bit of UX improvement for the cases where someone is calling
I know we're going to redesign these screens pretty soon so probably not worth a ton of effort but something small to help us cross the bridge for now? Maybe just a copy change saying something like: "Unable to determine token standard, name or symbol. Approving will grant allowance for this unidentifiable token to the following address." ? |
|
How about |
|
@rachelcope what do you think of something like this: |
|
Updated this fix to include additional context - a link to the contract on the blockexplorer - for the user to be sure they know what they are approving in cases where we don't have complete information: Decentraland (LAND): CryptoKitties (CK) ENS In my opinion this adds critical context with the link to allow users to verify the contract via the block-explorer as a backstop when we don't have name or symbol for the context on this screen. We should be adding the contract (both a link and legible address) but this is an improvement for now. I understand that @SayaGT has an improved approve screen in the works. |
adonesky1
force-pushed
the
fix-approve-infinite-spin
branch
from
9a983f1 to
6f2860a
Compare
Builds ready [6f2860a]Page Load Metrics (2081 ± 168 ms)
highlights:storybook
|
adonesky1
force-pushed
the
fix-approve-infinite-spin
branch
from
6f2860a to
aa19c32
Compare
Builds ready [aa19c32]Page Load Metrics (1852 ± 66 ms)
highlights:storybook
|
adonesky1
force-pushed
the
fix-approve-infinite-spin
branch
from
aa19c32 to
b4b70c0
Compare
Builds ready [b4b70c0]Page Load Metrics (1747 ± 55 ms)
highlights:storybook
|
|
Code looks good and works, but I"m wondering if there's a test we can add to prevent regressions, since the ERC logic is and symbol asset stuff presents many cases. |
|
Pour please 🍷 🙏 More than a few projects halted due to this issue and even if you rebuild it, I assume you'll want a stable/LTS legacy just incase because with changes like this it's breaking support for who knows how many projects. |
|
I think a test would be helpful -- do we know when that redesign is scheduled for? Even still, I think it could take months for that new UI to hit. |
|
I think additionally we should add e2e tests for this flow that include non-standard tokens |
|
@darkwing I actually don't think we should extract |
|
Added test coverage to partially cover us against regressions for this: useAssetDetails test but I am also working on adding non-standard tokens e2e tests, with some additions to test-dapp These tests I just added are throwing the |
… when name/symbol is not available
adonesky1
force-pushed
the
fix-approve-infinite-spin
branch
from
025f32a to
90b32ff
Compare
|
@adonesky1 It looks like you're |
Unfortunately I'm still getting the warning ( |
Builds ready [974ba83]Page Load Metrics (1688 ± 35 ms)
highlights:storybook
|
|
@adonesky1 |
|
I changed the first test to this and it seemed to work: it('should return object with tokenSymbol set to and empty string, when getAssetDetails returns and empty object', async () => {
const toAddress = '000000000000000000000000000000000000dead';
const tokenAddress = '0x1';
const transactionData = `0xa9059cbb000000000000000000000000${toAddress}000000000000000000000000000000000000000000000000016345785d8a0000`;
const { result, waitForNextUpdate } = renderUseAssetDetails({
tokenAddress,
userAddress: '0x111',
transactionData,
});
await waitForNextUpdate();
expect(result.current).toStrictEqual({
assetAddress: tokenAddress,
assetName: undefined,
assetStandard: undefined,
decimals: undefined,
toAddress: `0x${toAddress}`,
tokenAmount: undefined,
tokenId: undefined,
tokenImage: undefined,
tokenSymbol: '',
tokenValue: undefined,
userBalance: undefined,
});
}); |
😜 I meant to do that... whoops! Thanks for the call out! |
adonesky1
force-pushed
the
fix-approve-infinite-spin
branch
from
acf957d to
d8e17ef
Compare
|
|
@adonesky1 Nice! That matches what I was able to do locally :) |
Builds ready [d8e17ef]Page Load Metrics (1843 ± 37 ms)
highlights:storybook
|
| const collectibles = useSelector(getCollectibles); | ||
| const tokenList = useSelector(getTokenList); |
There was a problem hiding this comment.
tokens and tokenList were never used by this hook, they had been at one point while developing it initially, but can be safely removed.
| const tokenData = parseStandardTokenTransactionData(transactionData); | ||
| assetStandard = standard; | ||
| assetAddress = tokenAddress; | ||
| tokenSymbol = symbol; | ||
| tokenSymbol = symbol ?? ''; |
There was a problem hiding this comment.
this is the basic fix to the regression I introduced here some months back. With this tokenSymbol will never be undefined allowing the confirmApprove screen to load.
Fixes issue where screen for confirming
approve()method calls gets stuck in infinite loading (#14459 & #14589 & #14518). This is set to be partially addressed when the commit associated with this PR is merged into the extension. It will be further addressed something like this WIP PR (the exact form of which is still being discussed). Even with these fixes however, there are still contracts for which the issue will not be resolved. The ENS ERC721 contract for example does not implement either asymbol()orname()method the result of which the approve screen currently relies upon. This PR simply adds a fallback value of""for the symbol so that once we're done loading contract data the symbol will not be undefined no matter what, allowing us to move past the loading screen.This fixes a regression introduced in this PR: #13788