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: asset contractId use inconsistencies #5427

Merged
merged 1 commit into from
May 23, 2024
Merged

Conversation

fbwoolf
Copy link
Contributor

@fbwoolf fbwoolf commented May 22, 2024

Try out Leather build fcf8adaExtension build, Test report, Storybook, Chromatic

This PR coordinates using just the contractId in our asset model. I relocated some stacks specific utils to stacks-utils for now, I'm not sure why they were living in the ui directory. I have renamed them all to coordinate our use bc it was sometimes unclear what part(s) were in the code.

Anyway, I think this is better and cleans things up a bit.

*No issue to link, follow up work to: #5414

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Enhanced handling of contract and asset names with new utility functions.
  • Bug Fixes

    • Corrected references and variable names for better consistency and clarity in asset metadata handling.
  • Refactor

    • Renamed and updated several functions to improve semantic clarity and maintain consistency across the codebase.
    • Updated key attributes and function imports for better alignment with actual property names and improved performance.
  • Chores

    • Updated @leather-wallet/models version from "0.6.3" to "0.6.4".
    • Removed obsolete utility functions related to transaction parsing in documentation.

@fbwoolf fbwoolf requested review from kyranjamie and alter-eggo May 22, 2024 19:08
Copy link

coderabbitai bot commented May 22, 2024

Walkthrough

The changes encompass a comprehensive update aimed at improving code clarity and consistency. Key enhancements include function renaming for better understanding, introduction of new utility functions for contract and asset management, and adjustments to property references in various components and hooks. Additionally, the version of @leather-wallet/models has been incremented from "0.6.3" to "0.6.4".

Changes

File Path Change Summary
package.json Updated version of @leather-wallet/models to "0.6.4".
src/app/common/stacks-utils.ts
src/app/common/utils.ts
Added new utility functions and renamed existing ones for clarity.
src/app/features/activity-list/components/transaction-list/stacks-transaction/ft-transfer-item.tsx Updated function names and property references for consistency.
src/app/features/asset-list/stacks/sip10-token-asset-list/... Adjusted variable names and function calls to align with data structures.
src/app/query/common/alex-sdk/alex-sdk.hooks.ts
src/app/query/stacks/sip10/sip10-tokens.utils.ts
.../token-metadata/fungible-tokens/fungible-token-metadata.query.ts
Enhanced imports and logic for improved functionality.
src/app/ui/utils/README.md Removed legacy transaction-related utility functions, focusing on string manipulation utilities.

🐇
In the realm of code where changes brew,
Functions align, and imports renew.
With clarity sought, and names in line,
The tech world shines, a tale so fine.
Updates made, logic now clear,
A coding journey without a fear. 🐇


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>.
    • 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 generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @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.

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 as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration 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.

@@ -76,3 +77,80 @@ export function getSafeImageCanonicalUri(imageCanonicalUri: string, name: string
? imageCanonicalUri
: '';
}

/**
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These should all be relocated soon with the @leather-wallet/stacks work, but wanted to move them with other stacks specific utils for now. Let me know if that seems incorrect, but I'm not sure why they would be in the ui lib?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, ui lib doesn't seems as correct place for them
may be refactor it to fn declarations while here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll do this!

contractAssetName: assetName,
contractId,
contractName,
contractId: key,
Copy link
Contributor Author

@fbwoolf fbwoolf May 22, 2024

Choose a reason for hiding this comment

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

This was my mistake/bug here, I believed the contractId was the principal, which is incorrect. It is now set to the entire asset identifier string (fully qualified name) so it can be used in parts where needed.

@fbwoolf fbwoolf changed the title fix: asset contractid use inconsistencies fix: asset contractId use inconsistencies May 22, 2024
Copy link

@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: 0

Outside diff range and nitpick comments (1)
src/app/common/stacks-utils.ts (1)

96-96: Good practice of logging warnings when expected patterns are not met in utility functions.

Also applies to: 112-112, 136-136

Review Details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits Files that changed from the base of the PR and between 281e718 and 8ff1c44.
Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !pnpm-lock.yaml
Files selected for processing (15)
  • package.json (1 hunks)
  • src/app/common/stacks-utils.ts (2 hunks)
  • src/app/common/transactions/stacks/transaction.utils.ts (2 hunks)
  • src/app/common/utils.ts (1 hunks)
  • src/app/features/activity-list/components/transaction-list/stacks-transaction/ft-transfer-item.tsx (2 hunks)
  • src/app/features/asset-list/stacks/sip10-token-asset-list/sip10-token-asset-item.tsx (1 hunks)
  • src/app/features/asset-list/stacks/sip10-token-asset-list/sip10-token-asset-list-unsupported.tsx (1 hunks)
  • src/app/features/asset-list/stacks/sip10-token-asset-list/sip10-token-asset-list.tsx (2 hunks)
  • src/app/pages/send/send-crypto-asset-form/form/sip10/use-sip10-send-form.tsx (2 hunks)
  • src/app/query/common/alex-sdk/alex-sdk.hooks.ts (3 hunks)
  • src/app/query/stacks/sip10/sip10-tokens.utils.ts (2 hunks)
  • src/app/query/stacks/token-metadata/fungible-tokens/fungible-token-metadata.query.ts (3 hunks)
  • src/app/query/stacks/token-metadata/non-fungible-tokens/non-fungible-token-metadata.query.ts (2 hunks)
  • src/app/store/transactions/token-transfer.hooks.ts (2 hunks)
  • src/app/ui/utils/README.md (1 hunks)
Files skipped from review due to trivial changes (4)
  • package.json
  • src/app/features/asset-list/stacks/sip10-token-asset-list/sip10-token-asset-item.tsx
  • src/app/pages/send/send-crypto-asset-form/form/sip10/use-sip10-send-form.tsx
  • src/app/ui/utils/README.md
Additional comments not posted (18)
src/app/features/asset-list/stacks/sip10-token-asset-list/sip10-token-asset-list.tsx (2)

3-3: Import and usage of getPrincipalFromContractId are correctly implemented.


28-28: Change of key property from tokenName to name enhances clarity and consistency in list rendering.

src/app/features/asset-list/stacks/sip10-token-asset-list/sip10-token-asset-list-unsupported.tsx (1)

41-41: Update of key property from tokenName to name correctly aligns with PR objectives and improves list key management.

src/app/query/stacks/sip10/sip10-tokens.utils.ts (3)

6-7: Import and usage of getStacksContractIdStringParts and getPrincipalFromContractId are correctly implemented.


18-28: Use of getStacksContractIdStringParts in createSip10CryptoAssetInfo function is appropriate and aligns with PR objectives.


43-48: Changes in filterSip10Tokens function enhance clarity and correctness by using getPrincipalFromContractId.

src/app/query/stacks/token-metadata/non-fungible-tokens/non-fungible-token-metadata.query.ts (1)

4-4: Import and usage of getPrincipalFromContractId are correctly implemented in the query hook.

src/app/features/activity-list/components/transaction-list/stacks-transaction/ft-transfer-item.tsx (1)

14-14: Import and usage of getPrincipalFromContractId are correctly implemented in the FtTransferItem component.

src/app/query/stacks/token-metadata/fungible-tokens/fungible-token-metadata.query.ts (2)

8-9: Import and usage of getStacksContractIdStringParts and getPrincipalFromContractId are correctly implemented in the query hooks.


Line range hint 61-98: Changes in useGetFungibleTokensBalanceMetadataQuery and useGetFungibleTokensMetadataQuery enhance clarity and correctness by using getPrincipalFromContractId and getStacksContractIdStringParts.

src/app/query/common/alex-sdk/alex-sdk.hooks.ts (2)

13-13: Updated import to use getPrincipalFromContractId aligns with PR objectives to standardize contractId usage.


44-44: Correct usage of getPrincipalFromContractId ensures accurate principal derivation from contract addresses.

Also applies to: 71-71

src/app/common/transactions/stacks/transaction.utils.ts (2)

21-21: Updated import to use getStacksContractName aligns with PR objectives for clearer function naming.


76-76: Correct usage of getStacksContractName in getTxTitle enhances clarity in displaying contract names.

src/app/common/stacks-utils.ts (2)

6-6: Import of logger is appropriate for logging potential issues in utility functions.


81-156: Addition of utility functions for parsing Stacks contract identifiers enhances clarity and consistency in handling contract data.

src/app/store/transactions/token-transfer.hooks.ts (1)

24-24: Updated import and usage of getStacksContractIdStringParts correctly handles contract ID parts in token transfer scenarios.

Also applies to: 86-88

src/app/common/utils.ts (1)

259-259: Addition of getPrincipalFromContractId function aligns with PR objectives to standardize contractId usage.

@fbwoolf fbwoolf force-pushed the fix/asset-models-contractid branch from 8ff1c44 to f752f34 Compare May 22, 2024 23:54
Copy link

@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: 0

Review Details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits Files that changed from the base of the PR and between 8ff1c44 and f752f34.
Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !pnpm-lock.yaml
Files selected for processing (15)
  • package.json (1 hunks)
  • src/app/common/stacks-utils.ts (2 hunks)
  • src/app/common/transactions/stacks/transaction.utils.ts (2 hunks)
  • src/app/common/utils.ts (1 hunks)
  • src/app/features/activity-list/components/transaction-list/stacks-transaction/ft-transfer-item.tsx (2 hunks)
  • src/app/features/asset-list/stacks/sip10-token-asset-list/sip10-token-asset-item.tsx (1 hunks)
  • src/app/features/asset-list/stacks/sip10-token-asset-list/sip10-token-asset-list-unsupported.tsx (1 hunks)
  • src/app/features/asset-list/stacks/sip10-token-asset-list/sip10-token-asset-list.tsx (2 hunks)
  • src/app/pages/send/send-crypto-asset-form/form/sip10/use-sip10-send-form.tsx (2 hunks)
  • src/app/query/common/alex-sdk/alex-sdk.hooks.ts (3 hunks)
  • src/app/query/stacks/sip10/sip10-tokens.utils.ts (2 hunks)
  • src/app/query/stacks/token-metadata/fungible-tokens/fungible-token-metadata.query.ts (3 hunks)
  • src/app/query/stacks/token-metadata/non-fungible-tokens/non-fungible-token-metadata.query.ts (2 hunks)
  • src/app/store/transactions/token-transfer.hooks.ts (2 hunks)
  • src/app/ui/utils/README.md (1 hunks)
Files skipped from review as they are similar to previous changes (15)
  • package.json
  • src/app/common/stacks-utils.ts
  • src/app/common/transactions/stacks/transaction.utils.ts
  • src/app/common/utils.ts
  • src/app/features/activity-list/components/transaction-list/stacks-transaction/ft-transfer-item.tsx
  • src/app/features/asset-list/stacks/sip10-token-asset-list/sip10-token-asset-item.tsx
  • src/app/features/asset-list/stacks/sip10-token-asset-list/sip10-token-asset-list-unsupported.tsx
  • src/app/features/asset-list/stacks/sip10-token-asset-list/sip10-token-asset-list.tsx
  • src/app/pages/send/send-crypto-asset-form/form/sip10/use-sip10-send-form.tsx
  • src/app/query/common/alex-sdk/alex-sdk.hooks.ts
  • src/app/query/stacks/sip10/sip10-tokens.utils.ts
  • src/app/query/stacks/token-metadata/fungible-tokens/fungible-token-metadata.query.ts
  • src/app/query/stacks/token-metadata/non-fungible-tokens/non-fungible-token-metadata.query.ts
  • src/app/store/transactions/token-transfer.hooks.ts
  • src/app/ui/utils/README.md

Copy link
Contributor

@alter-eggo alter-eggo left a comment

Choose a reason for hiding this comment

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

tested with different sip10 tokens and all worked ok for me

@@ -76,3 +77,80 @@ export function getSafeImageCanonicalUri(imageCanonicalUri: string, name: string
? imageCanonicalUri
: '';
}

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, ui lib doesn't seems as correct place for them
may be refactor it to fn declarations while here?

@alter-eggo
Copy link
Contributor

@314159265359879 could you please test sip10 tokens send transfer in this pr as well

Copy link
Contributor

@314159265359879 314159265359879 left a comment

Choose a reason for hiding this comment

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

Tested sending SIP10 tokens with hardware and software extension with this build. LGTM

Copy link
Contributor

@edgarkhanzadian edgarkhanzadian left a comment

Choose a reason for hiding this comment

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

Great work! As soon as this PR gets merged, #5343 queries PR can get updated and merged too

@fbwoolf fbwoolf force-pushed the fix/asset-models-contractid branch from f752f34 to ae495cd Compare May 23, 2024 16:01
@fbwoolf fbwoolf force-pushed the fix/asset-models-contractid branch from ae495cd to fcf8ada Compare May 23, 2024 16:03
Copy link

@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: 0

Review Details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits Files that changed from the base of the PR and between f752f34 and fcf8ada.
Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !pnpm-lock.yaml
Files selected for processing (15)
  • package.json (1 hunks)
  • src/app/common/stacks-utils.ts (2 hunks)
  • src/app/common/transactions/stacks/transaction.utils.ts (2 hunks)
  • src/app/common/utils.ts (1 hunks)
  • src/app/features/activity-list/components/transaction-list/stacks-transaction/ft-transfer-item.tsx (2 hunks)
  • src/app/features/asset-list/stacks/sip10-token-asset-list/sip10-token-asset-item.tsx (1 hunks)
  • src/app/features/asset-list/stacks/sip10-token-asset-list/sip10-token-asset-list-unsupported.tsx (1 hunks)
  • src/app/features/asset-list/stacks/sip10-token-asset-list/sip10-token-asset-list.tsx (2 hunks)
  • src/app/pages/send/send-crypto-asset-form/form/sip10/use-sip10-send-form.tsx (2 hunks)
  • src/app/query/common/alex-sdk/alex-sdk.hooks.ts (3 hunks)
  • src/app/query/stacks/sip10/sip10-tokens.utils.ts (2 hunks)
  • src/app/query/stacks/token-metadata/fungible-tokens/fungible-token-metadata.query.ts (3 hunks)
  • src/app/query/stacks/token-metadata/non-fungible-tokens/non-fungible-token-metadata.query.ts (2 hunks)
  • src/app/store/transactions/token-transfer.hooks.ts (2 hunks)
  • src/app/ui/utils/README.md (1 hunks)
Files skipped from review as they are similar to previous changes (15)
  • package.json
  • src/app/common/stacks-utils.ts
  • src/app/common/transactions/stacks/transaction.utils.ts
  • src/app/common/utils.ts
  • src/app/features/activity-list/components/transaction-list/stacks-transaction/ft-transfer-item.tsx
  • src/app/features/asset-list/stacks/sip10-token-asset-list/sip10-token-asset-item.tsx
  • src/app/features/asset-list/stacks/sip10-token-asset-list/sip10-token-asset-list-unsupported.tsx
  • src/app/features/asset-list/stacks/sip10-token-asset-list/sip10-token-asset-list.tsx
  • src/app/pages/send/send-crypto-asset-form/form/sip10/use-sip10-send-form.tsx
  • src/app/query/common/alex-sdk/alex-sdk.hooks.ts
  • src/app/query/stacks/sip10/sip10-tokens.utils.ts
  • src/app/query/stacks/token-metadata/fungible-tokens/fungible-token-metadata.query.ts
  • src/app/query/stacks/token-metadata/non-fungible-tokens/non-fungible-token-metadata.query.ts
  • src/app/store/transactions/token-transfer.hooks.ts
  • src/app/ui/utils/README.md

@fbwoolf fbwoolf added this pull request to the merge queue May 23, 2024
Merged via the queue into dev with commit d023622 May 23, 2024
30 checks passed
@fbwoolf fbwoolf deleted the fix/asset-models-contractid branch May 23, 2024 17:39
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.

5 participants