Merged
Conversation
rAskVAL
force-pushed
the
SD-34-adapt-shielder-to-new-designs
branch
from
2be3123 to
9aeae72
Compare
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
rAskVAL
force-pushed
the
SD-34-adapt-shielder-to-new-designs
branch
from
9aeae72 to
715970d
Compare
rAskVAL
force-pushed
the
SD-34-adapt-shielder-to-new-designs
branch
from
715970d to
7cb2945
Compare
rAskVAL
force-pushed
the
SD-34-adapt-shielder-to-new-designs
branch
from
ac4126f to
172d7c2
Compare
rAskVAL
force-pushed
the
SD-34-adapt-shielder-to-new-designs
branch
from
172d7c2 to
66e6bc3
Compare
rAskVAL
force-pushed
the
SD-34-adapt-shielder-to-new-designs
branch
from
66e6bc3 to
d347522
Compare
rAskVAL
force-pushed
the
SD-34-adapt-shielder-to-new-designs
branch
from
7913cc3 to
df7ca36
Compare
rAskVAL
force-pushed
the
SD-34-adapt-shielder-to-new-designs
branch
from
98aeda5 to
cf4f6a0
Compare
rAskVAL
force-pushed
the
SD-34-adapt-shielder-to-new-designs
branch
from
cf4f6a0 to
fa37d67
Compare
rAskVAL
force-pushed
the
SD-34-adapt-shielder-to-new-designs
branch
from
fa37d67 to
6f6f6c4
Compare
rAskVAL
force-pushed
the
SD-34-adapt-shielder-to-new-designs
branch
from
6f6f6c4 to
80ba876
Compare
rAskVAL
force-pushed
the
SD-34-adapt-shielder-to-new-designs
branch
from
80ba876 to
906c8b5
Compare
jalooc
approved these changes
Apr 9, 2025
Contributor
jalooc
left a comment
jalooc
left a comment
There was a problem hiding this comment.
Nice and clean - amazing 🏆 🎖️
2 notes wrt the comments I've left:
- some of them relate to multiple occurrences of the issue in question - for each issue I have left a comment only on one occurrence of them, so please make sure you find and address all,
- you were pushing like crazy during my review, so many of the files I commented on are outdated - I'm sure you'll manage to map the comments to the new code ;)
src/domains/misc/components/InteractiveSlider/utils/useOvershootLimitedSpringMotion.ts
Outdated
Show resolved
Hide resolved
| tokenDecimals: (tokenAddress: Address) => ['token-decimals', tokenAddress] as const, | ||
| tokenName: (tokenAddress: Address) => ['token-name', tokenAddress] as const, | ||
| tokenSymbol: (tokenAddress: Address) => ['token-symbol', tokenAddress] as const, | ||
| tokenDecimals: (tokenAddress: Address | 'native') => ['token-decimals', tokenAddress] as const, |
Contributor
There was a problem hiding this comment.
In other apps we're doing null for native. I dislike using string | 'specific-string' because such a union always evaluate as just string, which makes the type less readable and easier to make a mistake.
But if it's too much work to change, then don't bother.
Contributor
Author
There was a problem hiding this comment.
Ill create ticket, it makes sense to use null.
| if (!walletAddress || !chainId) return; | ||
|
|
||
| console.error('Shielding failed:', error); | ||
| void queryClient.invalidateQueries({ |
Contributor
There was a problem hiding this comment.
Invalidation on onError? Shouldn't this be onSuccess?
Contributor
Author
There was a problem hiding this comment.
Sometimes if it fails fee still can be taken, so I refetch native balance.
Contributor
There was a problem hiding this comment.
Ouukay. Maybe leave a comment with that information then?
rAskVAL
force-pushed
the
SD-34-adapt-shielder-to-new-designs
branch
from
906c8b5 to
f9d9d8f
Compare
rAskVAL
force-pushed
the
SD-34-adapt-shielder-to-new-designs
branch
from
f9d9d8f to
7692cb9
Compare
rAskVAL
force-pushed
the
SD-34-adapt-shielder-to-new-designs
branch
from
7692cb9 to
54c5cb1
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.