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

🧹 (Dust to OSMO): Feature implementation #3913

Open
wants to merge 15 commits into
base: stage
Choose a base branch
from

Conversation

greg-nagy
Copy link
Contributor

@greg-nagy greg-nagy commented Oct 25, 2024

What is the purpose of the change:

This PR is the v0 for the dust to OSMO feature.

Outline for the solution

  • add a Dust to OSMO button to the portfolio page under the asset table
  • when the user clicks Swap dust to OSMO
    • loading spinner is added to the button
    • a swap is generated for every asset, and swaps are executed sequentially
    • user is prompted to sign every tx by the wallet
    • after all swap txs resulted in either success or error remove the loading spinner

Linear Task

Brief for the task

Currently, a user who regularly trades on the platform may have a number of small balances remaining in their portfolio due to the values of various tokens being incommensurate. These small remaining balances are known as 'dust' and often sit unused since the value is too low in order to justify transacting due to network fees. Your task is to write a function that converts the dust in a wallet (defined as values less than $0.01USD) to OSMO.

Note that there are no designs for this task. You may either write a well tested function for review, or use your best judgement in adding UX to trigger the function.

The task will involve reading and understanding how balances are fetched in the codebase, and understanding signing broadcasting transactions.

Brief Changelog

  • add a feature flag
  • add the dust to osmo button
  • update useSwap to be easier to use programatically
  • add getUserDustAsset() procedure
  • on user action execute all the swaps sequentially either with manual signing or 1click trading
  • add react query devtools for debugging
  • add a generic query client for non trpc async handling

Testing and Verifying

This change has been tested locally by rebuilding the website and verified content and links are expected

  • set NEXT_PUBLIC_SHOW_DUST_TO_OSMO=true in packages/web/.env.local
  • go to portfolio
  • make sure you have some dust in the currently connected wallet
  • (1click trading off): click Dust to OSMO button
  • (1click trading on): click Dust to OSMO button

Backlog

  • set up the flag in launch darkly
  • drop the environment variable from .env and env.ts and hardcode a default value in web/hooks/use-feature-flags.ts
  • Resolve the TODO(Greg) comments

To be discussed

  • TODO(Greg) comments
  • Do we want an overview, confirmation, and error display user interface?
  • Should we batch the swaps into a single tx so only 1 signature is necessary?
  • Do we want to reduce dust sweep costs?

Demo

manual sign

CleanShot 2024-11-05 at 09 09 36

1click trade

CleanShot 2024-11-05 at 09 15 59

Copy link

vercel bot commented Oct 25, 2024

@greg-nagy is attempting to deploy a commit to the OsmoLabs Team on Vercel.

A member of the Team first needs to authorize it.

@MaxMillington
Copy link
Collaborator

Hi @greg-nagy (apologies for the slow reply). I think for this task, something simpler is in order. A button that triggers swaps for all assets that have values less than $0.01 USD is what we are looking for. The swaps should also be converted to OSMO

this query client can be used for all async tasks in the app

this client is probably best to be separated from the trpc query client
(trpc query client has persistance, logging, 24h cache etc)
when using the useSwap programatically the user input debounce is a
non-concern, but it's important when dealing with user input from the UI

the debounced version of the inAmountInput and outAmountInput are used
for fetching the quotes and the quotes are essential downstream
it's not trivial when swap is ready to be executed because useSwap has a
lot to take care for

this simplifies the usage for the consumer
@greg-nagy greg-nagy force-pushed the dust-to-osmo branch 2 times, most recently from 1517394 to ccb918d Compare November 4, 2024 15:39
Reusing the existing infrastructure for swaps encoded in the useSwap
seemed to be the favorable direction.

The main trick to it is to wrap the hooks into lightweight components
and handle the batching through the component interactions.

(implmeneting `useDustToOsmo` from scratch would have been possible but
it would have needed more testing and it would have duplicated at least
some of the existing functionality — eg handling 1 click trading)
calculating and prepopulating the sequence would be possible but didn't
seem feasible (it's coded in the stores -> account -> base ->
signAndBroadcast => sign => getSequence which is widely used and pretty
deep in layers)

The other options would have been to batch all the swaps into a single
tx. This is a good option but it's out of the scope of the first PR and
would require the reimplementation of the `sendTradeTokenInTx` for batch
sending
otherwise SequentialSwapExecutor > hasAllSwapsFinished will be always
false > the onComplete won't be called

we are clicking back and forth from the wallet signing ui -> on every
refocus the query will be refetched by default -> in the new list we
will have less dust assets (because we have swapped them back to osmo)
a bit of time is necessary beetween the broadcast and starting of the
next swap to avoid account seq mismatches

the swap debounce time is perfect for this purpose
without this refresh the last swapped asset stays in the table until a
getUserBridgeAsset() refetch
and only start dustConversion if there is actually dust
after a dust sweep if the button is pressed again then the last stale
element will flicker on to the screen and we don't want that
@greg-nagy greg-nagy changed the title 🧹 (Dust to OSMO): Init feature and design discussion 🧹 (Dust to OSMO): Feature implementation Nov 5, 2024
@greg-nagy greg-nagy marked this pull request as ready for review November 5, 2024 08:26
Copy link
Contributor

coderabbitai bot commented Nov 5, 2024

Walkthrough

This pull request introduces several features and modifications across multiple files. A new method getUserDustAssets is added to the assetsRouter, allowing users to retrieve assets with a USD value below a specified threshold. Additionally, a new React component DustToOsmo is created for converting "dust" assets into OSMO tokens. The environment variable configuration is updated, and a new feature flag for dustToOsmo is introduced. Various components and hooks are modified to integrate these new features, enhancing the overall functionality related to asset management.

Changes

File Change Summary
packages/trpc/src/assets.ts Added method getUserDustAssets to assetsRouter; includes input schema merging UserOsmoAddressSchema with dustThreshold.
packages/types/src/feature-flags-types.ts Updated AvailableFlags type to include new flag "dustToOsmo".
packages/web/.env Added environment variable NEXT_PUBLIC_SPEND_LIMIT_CONTRACT_ADDRESS; activated and commented out NEXT_PUBLIC_SHOW_DUST_TO_OSMO.
packages/web/components/complex/dust-to-osmo/dust-to-osmo.tsx Introduced DustToOsmo, SequentialSwapExecutor, and SwapHandler components for dust conversion functionality.
packages/web/components/table/portfolio-asset-balances.tsx Integrated DustToOsmo component; added featureFlags variable for conditional rendering; updated layout and added refetch method.
packages/web/config/env.ts Added constant SHOW_DUST_TO_OSMO based on environment variable.
packages/web/hooks/use-feature-flags.ts Added dustToOsmo flag initialized with SHOW_DUST_TO_OSMO in defaultFlags.
packages/web/hooks/use-swap.tsx Enhanced SwapOptions type and useSwap function with new property inputDebounceMs; refined swap readiness logic.
packages/web/pages/_app.tsx Added QueryClient and QueryClientProvider for state management; integrated ReactQueryDevtools.
packages/web/utils/trpc.ts Introduced queryClientTRPC for centralized query management; updated persistQueryClient function.

Possibly related PRs

Suggested reviewers

  • MaxMillington
  • JoseRFelix
  • CryptoAssassin1

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (12)
packages/web/config/env.ts (2)

29-31: Consider tracking the LaunchDarkly integration as a technical debt item.

The TODO comment indicates this is a temporary solution. To ensure this technical debt is properly tracked and addressed:

  1. Consider creating a GitHub issue for the LaunchDarkly integration
  2. Add the issue number to the TODO comment
  3. Document any specific requirements or considerations for the migration

Would you like me to help create a GitHub issue to track this technical debt?


28-31: Consider grouping feature flags together.

For better maintainability, consider grouping all feature flag-related constants together in a dedicated section of the file or in a separate feature flags configuration file.

Example structure:

// Feature Flags
export const SHOW_DUST_TO_OSMO = process.env.NEXT_PUBLIC_SHOW_DUST_TO_OSMO === "true";
// Add other feature flags here
packages/web/hooks/use-feature-flags.ts (1)

5-5: Consider marking this import as temporary.

This import represents a temporary solution until the feature flag is moved to LaunchDarkly. Consider adding a // TEMPORARY comment to make this clear to other developers.

-import { SHOW_DUST_TO_OSMO } from "~/config";
+// TEMPORARY: Remove once dustToOsmo flag is moved to LaunchDarkly
+import { SHOW_DUST_TO_OSMO } from "~/config";
packages/web/utils/trpc.ts (1)

45-51: LGTM! Consider adding explicit type annotation.

The extraction of queryClientTRPC as a shared constant is a good practice. Consider adding an explicit type annotation for better type safety:

-export const queryClientTRPC = new QueryClient({
+export const queryClientTRPC: QueryClient = new QueryClient({
packages/trpc/src/assets.ts (2)

415-423: Simplify Dec validation using transform.

The custom validation for Dec can be simplified using zod's transform feature.

-          dustThreshold: z.custom<Dec>((val) => {
-            if (val instanceof Dec) return true;
-            try {
-              new Dec(val as string | number);
-              return true;
-            } catch {
-              return false;
-            }
-          }, "Invalid Dec value"),
+          dustThreshold: z.union([
+            z.instanceof(Dec),
+            z.string().or(z.number()).transform((val) => new Dec(val))
+          ]),

436-442: Replace type assertion with type guard function.

The type assertion in the filter could be replaced with a more maintainable type guard function.

+      function hasAmountAndUsdValue(asset: typeof assets[number]): asset is 
+        Omit<typeof asset, "amount" | "usdValue"> &
+        Required<Pick<typeof asset, "amount" | "usdValue">> {
+        return asset.amount !== undefined && asset.usdValue !== undefined;
+      }
+
       const assetsWithAmountAndUsdValue = assets.filter(
-        (
-          asset
-        ): asset is Omit<typeof asset, "amount" | "usdValue"> &
-          Required<Pick<typeof asset, "amount" | "usdValue">> =>
-          asset.amount !== undefined && asset.usdValue !== undefined
+        hasAmountAndUsdValue
       );
packages/web/components/table/portfolio-asset-balances.tsx (2)

444-448: Consider adding ARIA labels for better accessibility

While the UI implementation is clean, consider enhancing accessibility by adding appropriate ARIA labels to the buttons container and its interactive elements.

-        <div className="flex gap-4 py-2 px-4">
+        <div className="flex gap-4 py-2 px-4" role="toolbar" aria-label="Portfolio actions">
           {featureFlags.dustToOsmo && <DustToOsmo onComplete={refetch} />}
           <Button
             onClick={() => setHideDust(!hideDust)}
             className="ml-auto gap-2 !border !border-osmoverse-700 !py-2 !px-4 !text-wosmongton-200"
             variant="outline"
             size="lg-full"
+            aria-label={hideDust ? t("portfolio.showHidden", { hiddenDustCount: hiddenDustCount.toString() }) : t("portfolio.hideSmallBalances")}

Line range hint 601-607: Implementation needed for conversion modal

The conversion button click handler is using a placeholder alert. This needs to be implemented to open the conversion modal.

Would you like me to help implement the conversion modal functionality or create a GitHub issue to track this task?

packages/web/hooks/use-swap.tsx (1)

724-738: Consider refactoring isReadyToSwap condition for better readability.

While the logic is correct, the condition could be more maintainable by grouping related checks.

Consider refactoring like this:

-  const isReadyToSwap =
-    maxSlippage?.isPositive() &&
-    account &&
-    swapAssets.fromAsset &&
-    swapAssets.toAsset &&
-    inAmount?.isPositive() &&
-    inDebouncedAmount?.isPositive() &&
-    inAmount?.equals(inDebouncedAmount) &&
-    outAmount?.isPositive() &&
-    outDebouncedAmount?.isPositive() &&
-    outAmount?.equals(outDebouncedAmount) &&
-    quoteToReturn &&
-    !precedentError &&
-    !hasOverSpendLimitError &&
-    !isSlippageOverBalance;
+  const hasRequiredContext = maxSlippage?.isPositive() && account;
+  const hasValidAssets = swapAssets.fromAsset && swapAssets.toAsset;
+  const hasValidInAmount = inAmount?.isPositive() && 
+    inDebouncedAmount?.isPositive() && 
+    inAmount?.equals(inDebouncedAmount);
+  const hasValidOutAmount = outAmount?.isPositive() && 
+    outDebouncedAmount?.isPositive() && 
+    outAmount?.equals(outDebouncedAmount);
+  const hasNoErrors = !precedentError && 
+    !hasOverSpendLimitError && 
+    !isSlippageOverBalance;
+
+  const isReadyToSwap = hasRequiredContext && 
+    hasValidAssets && 
+    hasValidInAmount && 
+    hasValidOutAmount && 
+    quoteToReturn &&
+    hasNoErrors;
packages/web/components/complex/dust-to-osmo/dust-to-osmo.tsx (1)

51-51: Use optional chaining for optional callbacks

To make the code more concise and idiomatic, consider using optional chaining when invoking the onComplete callback.

Apply this diff to simplify the function call:

-        onComplete && onComplete();
+        onComplete?.();
🧰 Tools
🪛 Biome

[error] 51-51: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

packages/web/pages/_app.tsx (2)

78-83: Address the TODO regarding usage of TanStack Query

The code contains a TODO about considering the use of TanStack Query for asynchronous operations. If you need assistance evaluating this decision or implementing it, I can help provide guidance or code examples.


89-93: Ensure consistent QueryClient usage between Provider and DevTools

The QueryClientProvider is initialized with queryClientGeneric, but the ReactQueryDevtools is configured with queryClientTRPC. This might lead to confusion or inconsistencies in state management. Consider consolidating to a single QueryClient instance for both the provider and the devtools to ensure consistent caching and state across your application.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between b828230 and fc1e4b6.

⛔ Files ignored due to path filters (19)
  • packages/web/localizations/de.json is excluded by !**/*.json
  • packages/web/localizations/en.json is excluded by !**/*.json
  • packages/web/localizations/es.json is excluded by !**/*.json
  • packages/web/localizations/fa.json is excluded by !**/*.json
  • packages/web/localizations/fr.json is excluded by !**/*.json
  • packages/web/localizations/gu.json is excluded by !**/*.json
  • packages/web/localizations/hi.json is excluded by !**/*.json
  • packages/web/localizations/ja.json is excluded by !**/*.json
  • packages/web/localizations/ko.json is excluded by !**/*.json
  • packages/web/localizations/pl.json is excluded by !**/*.json
  • packages/web/localizations/pt-br.json is excluded by !**/*.json
  • packages/web/localizations/ro.json is excluded by !**/*.json
  • packages/web/localizations/ru.json is excluded by !**/*.json
  • packages/web/localizations/tr.json is excluded by !**/*.json
  • packages/web/localizations/zh-cn.json is excluded by !**/*.json
  • packages/web/localizations/zh-hk.json is excluded by !**/*.json
  • packages/web/localizations/zh-tw.json is excluded by !**/*.json
  • packages/web/package.json is excluded by !**/*.json
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock, !**/*.lock
📒 Files selected for processing (10)
  • packages/trpc/src/assets.ts (2 hunks)
  • packages/types/src/feature-flags-types.ts (1 hunks)
  • packages/web/.env (1 hunks)
  • packages/web/components/complex/dust-to-osmo/dust-to-osmo.tsx (1 hunks)
  • packages/web/components/table/portfolio-asset-balances.tsx (4 hunks)
  • packages/web/config/env.ts (1 hunks)
  • packages/web/hooks/use-feature-flags.ts (2 hunks)
  • packages/web/hooks/use-swap.tsx (7 hunks)
  • packages/web/pages/_app.tsx (3 hunks)
  • packages/web/utils/trpc.ts (3 hunks)
🧰 Additional context used
🪛 Biome
packages/web/components/complex/dust-to-osmo/dust-to-osmo.tsx

[error] 51-51: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

🔇 Additional comments (16)
packages/types/src/feature-flags-types.ts (2)

28-29: LGTM! Feature flag addition is appropriate.

The addition of the "dustToOsmo" feature flag is well-placed and follows naming conventions. Given the financial nature of dust conversion and transaction signing requirements, having a feature flag is crucial for controlled rollout.


29-29: Verify feature flag usage across components.

Let's ensure the feature flag is properly integrated across all relevant components.

✅ Verification successful

Feature flag integration is properly implemented

The feature flag dustToOsmo is correctly integrated across all necessary components:

  • Defined in feature flags types
  • Implemented in use-feature-flags.ts hook
  • Environment variable NEXT_PUBLIC_SHOW_DUST_TO_OSMO properly configured
  • Used conditionally in portfolio-asset-balances.tsx to render the DustToOsmo component
  • Component implementation in place with proper localization support
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the feature flag usage across components

# Test 1: Check direct usage of the feature flag
echo "Checking direct feature flag usage:"
rg -l "dustToOsmo" --type ts --type tsx

# Test 2: Check environment variable usage
echo -e "\nChecking environment variable usage:"
rg -l "NEXT_PUBLIC_SHOW_DUST_TO_OSMO" --type ts --type tsx

# Test 3: Check DustToOsmo component imports
echo -e "\nChecking DustToOsmo component usage:"
rg -l "DustToOsmo" --type ts --type tsx

Length of output: 481


Script:

#!/bin/bash
# Let's try without specifying file types since tsx wasn't recognized
# Instead use general pattern matching for TypeScript/React files

# Test 1: Check direct usage of the feature flag
echo "Checking direct feature flag usage:"
rg -l "dustToOsmo"

# Test 2: Check environment variable usage
echo -e "\nChecking environment variable usage:"
rg -l "NEXT_PUBLIC_SHOW_DUST_TO_OSMO"

# Test 3: Check DustToOsmo component usage
echo -e "\nChecking DustToOsmo component usage:"
rg -l "DustToOsmo"

# Test 4: Let's also check the feature flags configuration
echo -e "\nChecking feature flags configuration:"
find . -type f -name "feature-flags*" -exec cat {} \;

Length of output: 2105


Script:

#!/bin/bash
# Let's check the actual implementation and usage of the feature flag

# Check the feature flag implementation in use-feature-flags.ts
echo "Checking feature flag implementation:"
rg -A 5 "dustToOsmo" packages/web/hooks/use-feature-flags.ts

# Check how it's used in portfolio-asset-balances.tsx
echo -e "\nChecking usage in portfolio balances:"
rg -A 5 "dustToOsmo" packages/web/components/table/portfolio-asset-balances.tsx

# Check the DustToOsmo component implementation
echo -e "\nChecking DustToOsmo component implementation:"
rg -A 5 "dustToOsmo" packages/web/components/complex/dust-to-osmo/dust-to-osmo.tsx

# Check environment configuration
echo -e "\nChecking environment configuration:"
rg -A 5 "NEXT_PUBLIC_SHOW_DUST_TO_OSMO" packages/web/config/env.ts

Length of output: 1438

packages/web/config/env.ts (1)

30-31: LGTM! The implementation follows established patterns.

The constant follows the existing boolean environment variable pattern used throughout the file.

packages/web/.env (2)

53-54: LGTM! Well-structured feature flag implementation.

The feature flag is properly commented out by default and follows the established naming convention. The descriptive comment clearly indicates its purpose.


51-52: Move contract address to environment-specific configuration files.

The contract address should not be hardcoded in the default .env file as it may vary across different environments (testnet, mainnet, etc.). Consider moving this to environment-specific files like .env.production or .env.development.

Let's check if this contract address is used in other environment files:

packages/web/hooks/use-feature-flags.ts (1)

36-37: Enhance TODO comment with context and tracking.

While the TODO correctly identifies the need to move this flag to LaunchDarkly, consider:

  1. Using LaunchDarkly would enable:

    • Real-time feature toggling without deployments
    • A/B testing capabilities
    • Gradual rollout to users
  2. This technical debt should be tracked properly.

Let's check if there's already an issue tracking this:

Consider enhancing the comment:

-  // TODO(Greg): add this flag to LaunchDarkly and drop the .env config
+  // TODO(Greg): Move dustToOsmo flag to LaunchDarkly to enable gradual rollout
+  // and real-time toggling. Remove SHOW_DUST_TO_OSMO from .env after migration.
+  // Tracking issue: <add issue link>
packages/web/utils/trpc.ts (1)

75-75: LGTM! Clean replacement of query client reference.

The update to use the shared queryClientTRPC instance is clean and maintains all the existing persistence logic.

packages/trpc/src/assets.ts (2)

411-447: Integration looks good!

The new procedure follows existing patterns and properly integrates with the codebase by reusing helper functions and maintaining consistency with other procedures.


431-432: Verify filtering assumption.

The TODO comment indicates uncertainty about the filtering assumption. This should be verified and documented.

✅ Verification successful

Let me gather more context about the asset verification logic.


Let me check one more aspect to understand how the verification status is used in queries.


Based on the gathered evidence, I can now provide a final response:

Filtering by onlyVerified: true is correct and validated by tests

The filtering assumption is correct as evidenced by:

  • Multiple consistent usages of onlyVerified: true across the codebase in asset-related queries
  • Test assertions explicitly verifying this behavior: expect(assets.some((asset) => !asset.isVerified)).toBeFalsy()
  • Asset interface includes isVerified as a required property
  • Test fixtures demonstrate verified assets having isVerified: true
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if other parts of the codebase filter by verified status
# when dealing with user assets

# Test: Look for patterns of asset filtering in the codebase
rg -A 3 "onlyVerified: true" --type ts

Length of output: 1485


Script:

#!/bin/bash
# Check the implementation of asset verification and filtering
ast-grep --pattern 'interface $_ {
  $$$
  isVerified: $_
  $$$
}'

# Check tests related to asset verification
rg -B 2 -A 2 "isVerified" "packages/server/src/queries/complex/assets/__tests__/assets.spec.ts"

# Check the asset list implementation
ast-grep --pattern 'function getAssetFromAssetList($_) {
  $$$
}'

Length of output: 4432


Script:

#!/bin/bash
# Check the implementation of asset queries and how verification is used
ast-grep --pattern 'function getAssets($_) {
  $$$
}'

# Look for asset filtering logic
rg -B 2 -A 4 "filterAssets" --type ts

Length of output: 93

packages/web/components/table/portfolio-asset-balances.tsx (3)

24-24: LGTM: Clean import of DustToOsmo component

The import is properly placed with other component imports.

Also applies to: 24-24


129-129: LGTM: Added refetch for data synchronization

The refetch functionality ensures the table data is refreshed after dust conversion, maintaining data consistency.

Also applies to: 129-129


445-445: Verify dust threshold implementation

Let's verify that the DustToOsmo component is using the correct dust threshold value ($0.01 USD as mentioned in PR objectives).

packages/web/hooks/use-swap.tsx (2)

78-80: LGTM: Well-documented type addition.

The new optional inputDebounceMs property is properly typed and documented, providing flexibility in controlling input debounce timing.


1019-1019: LGTM: Consistent implementation of debounce customization.

The changes properly implement the debounce customization feature with a consistent default value of 200ms.

Also applies to: 1024-1024, 1037-1037

packages/web/components/complex/dust-to-osmo/dust-to-osmo.tsx (2)

69-69: Possible typo in className

The className text-wosmongton-200 might contain a typo. Please verify if wosmongton is the intended value or if it should be corrected.

Would you like me to check for the correct class name in the codebase?


33-35: Clarify the dust threshold value

There's a TODO comment questioning whether to use $0.01 or $0.02 as the dust threshold. To ensure consistency and meet the specified requirements, please confirm the correct threshold value and update the code accordingly.

I can help verify the defined value of HideDustUserSetting.DUST_THRESHOLD in the codebase. Here's a script to find its definition:

Would you like assistance in updating the threshold value based on the findings?

✅ Verification successful

The dust threshold is correctly set to 0.02

The code is already using the correct dust threshold value. The constant HideDustUserSetting.DUST_THRESHOLD is defined in packages/web/stores/user-settings/hide-dust.tsx with a value of 0.02, which matches the updated requirement mentioned in the TODO comment.

The TODO comment can be safely removed as it's no longer needed.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find the definition of HideDustUserSetting.DUST_THRESHOLD

# Expected result: Display the lines where DUST_THRESHOLD is defined.
rg 'DUST_THRESHOLD' -A 2

Length of output: 812

@@ -97,7 +97,7 @@ export const api = createTRPCNext<AppRouter>({
});

return {
queryClient,
queryClient: queryClientTRPC,
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 5, 2024

Choose a reason for hiding this comment

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

💡 Codebase verification

Integration issue detected: QueryClientProvider using different client instance

The verification revealed a potential issue in the integration:

  • queryClientTRPC is correctly exported and defined in utils/trpc.ts
  • However, in _app.tsx, the QueryClientProvider is using a different client instance queryClientGeneric
  • queryClientTRPC is only being used in the ReactQueryDevtools component

This mismatch between the query client instances could lead to synchronization issues. The QueryClientProvider should be using queryClientTRPC to maintain consistency.

🔗 Analysis chain

LGTM! Verify integration with providers.

The update to use the shared queryClientTRPC instance is correct. Let's verify its integration with QueryClientProvider.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that queryClientTRPC is properly integrated with QueryClientProvider
# Test: Search for QueryClientProvider usage with queryClientTRPC
rg -A 3 "QueryClientProvider.*queryClientTRPC" packages/web/pages/_app.tsx

Length of output: 76


Script:

#!/bin/bash
# Let's first check if queryClientTRPC is defined and exported
rg -A 3 "export.*queryClientTRPC" packages/web

# Then check for any QueryClientProvider usage in _app.tsx
rg -A 3 "QueryClientProvider" packages/web/pages/_app.tsx

# Also check for any other files that might be using QueryClientProvider
rg -A 3 "QueryClientProvider" packages/web

# Finally, let's see how queryClientTRPC is being used
rg -A 3 "queryClientTRPC" packages/web

Length of output: 3637

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Valid point, I left a question about this with TODO(Greg) in the _app.tsx with some thoughts.

I considered this a good way forward, but we also have other options.

Copy link
Contributor

Choose a reason for hiding this comment

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

@greg-nagy, understood. I see that you've left a TODO in _app.tsx with your thoughts. Let me know if I can assist further.

(_/)
(•_•)
/ >📖

Comment on lines +444 to +446
return assetsWithAmountAndUsdValue.filter((asset) =>
asset.usdValue.toDec()?.lte(dustThreshold)
);
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 5, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add error handling for asset price fetching.

The filter assumes usdValue is always valid when defined. Consider adding error handling for potential failures in price fetching.

       return assetsWithAmountAndUsdValue.filter((asset) =>
-        asset.usdValue.toDec()?.lte(dustThreshold)
+        asset.usdValue.toDec().lte(dustThreshold)
       );

Also, consider adding a try-catch block to handle potential price fetching errors:

+      const filteredAssets = [];
+      for (const asset of assetsWithAmountAndUsdValue) {
+        try {
+          if (asset.usdValue.toDec().lte(dustThreshold)) {
+            filteredAssets.push(asset);
+          }
+        } catch (error) {
+          console.error(`Failed to process asset ${asset.coinDenom}:`, error);
+          continue;
+        }
+      }
+      return filteredAssets;

Committable suggestion skipped: line range outside the PR's diff.

Copy link
Contributor Author

@greg-nagy greg-nagy Nov 5, 2024

Choose a reason for hiding this comment

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

The filter assumes usdValue is always valid when defined. Consider adding error handling for potential failures in price fetching.

There is a filter above this that makes sure this is true. (The filter doesn't check the type assumption that the values are indeed CoinPretty or PricePretty)

Also, consider adding a try-catch block to handle potential price fetching errors:

The errors are ok to flow through the client.

Copy link
Contributor

Choose a reason for hiding this comment

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

@greg-nagy, thank you for the clarification. I understand that the filter above ensures usdValue is valid when defined, and that it's acceptable for errors to flow through to the client in this context. I'll keep this in mind for future reviews.


✏️ Learnings added
Learnt from: greg-nagy
PR: osmosis-labs/osmosis-frontend#3913
File: packages/trpc/src/assets.ts:444-446
Timestamp: 2024-11-05T08:46:24.588Z
Learning: In `packages/trpc/src/assets.ts`, within the `getUserDustAssets` procedure, a prior filter ensures that `usdValue` is defined and valid, so additional error handling for `usdValue` may not be necessary in subsequent operations.

Learnt from: greg-nagy
PR: osmosis-labs/osmosis-frontend#3913
File: packages/trpc/src/assets.ts:444-446
Timestamp: 2024-11-05T08:46:24.588Z
Learning: In the codebase, it's acceptable for errors to propagate to the client from the `getUserDustAssets` procedure in `packages/trpc/src/assets.ts`, so adding try-catch blocks for handling potential price fetching errors is not required.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

@greg-nagy
Copy link
Contributor Author

@MaxMillington I have finished what I had in mind for this PR

Lloyd emailed me about a bug that could impact the usability of this feature. I’m unsure about the details, but I think it’s worth sharing my progress so you can decide if it adds value. If it does, great; if not, we can close the PR.

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.

2 participants