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.

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 wrong native asset being displayed on SignTransactionSheet #6395

Merged
merged 3 commits into from
Jan 15, 2025

Conversation

walmat
Copy link
Contributor

@walmat walmat commented Jan 10, 2025

Fixes APP-2256

What changed (plus any additional context for devs)

1. Props Interface Update

Type Changes

  • Removed walletBalance prop interface with multiple fields
  • Replaced with nativeAsset prop using ParsedAddressAsset type
  • Updated component to use new prop structure throughout

2. Native Asset Handling

Implementation Changes

  • Modified native asset retrieval logic in SignTransactionSheet
  • Added fallback to getChainsNativeAsset when network native asset is unavailable
  • Improved type safety with proper null checks and fallbacks

Balance Handling

  • Added conditional check for object type and 'balance' property
  • Implemented better fallback values for balance display
  • Maintained backward compatibility with existing symbol display

3. Code Optimization

Dependency Updates

  • Reordered useMemo dependencies for better performance
  • Removed redundant dependencies from dependency arrays
  • Simplified string template usage for balance display

Error Messages

  • Updated error message handling to use new native asset structure
  • Maintained consistent symbol usage across error states
  • Improved i18n implementation for native token messages

This PR represents a significant improvement in how native assets are handled in transaction simulations, providing better type safety and more robust fallback behavior while maintaining existing functionality.

Screen recordings / screenshots

Screenshot 2025-01-14 at 1 43 01 PM

What to test

test eth natives and non-eth natives

@brunobar79
Copy link
Member

Launch in simulator or device for 602b9bb

@derHowie
Copy link
Member

hey @walmat can you throw a quick screen cap on here pls

Comment on lines +110 to +111
const nativeAsset =
ethereumUtils.getNetworkNativeAsset({ chainId }) ?? useBackendNetworksStore.getState().getChainsNativeAsset()[chainId];
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, is this fallback needed for this specific situation or should it be done like this in other places where you need the native asset? If the latter would be nice to add it directly to the ethereumUtils.getNetworkNativeAsset so this doesn't have to be re-done in other places.

Copy link
Contributor Author

@walmat walmat Jan 15, 2025

Choose a reason for hiding this comment

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

I think it depends on the scenario. The first call only grabs the native asset (with balance), and the second just grabs the base info from the backend. In this case, we can utilize both, but some places require it with balance so it doesn't necessarily make sense to have the fallback

Comment on lines +141 to +144
amount: nativeAsset?.balance?.amount || 0,
display: nativeAsset?.balance?.display || `0 ${nativeAsset?.symbol}`,
isLoaded: nativeAsset?.balance?.display !== undefined,
symbol: nativeAsset?.symbol || 'ETH',
Copy link
Contributor

Choose a reason for hiding this comment

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

these optional chains aren't necessary given the condition

@walmat walmat merged commit 39a3775 into develop Jan 15, 2025
11 checks passed
@walmat walmat deleted the @matthew/APP-2256 branch January 15, 2025 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants