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.

Sign up for GitHub

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

Jump to bottom

refactor: post condition asset info #1754

Merged
merged 3 commits into from Oct 1, 2021
Merged

refactor: post condition asset info #1754

merged 3 commits into from Oct 1, 2021

Conversation

fbwoolf
Copy link
Contributor

@fbwoolf fbwoolf commented Sep 28, 2021 •
edited by github-actions bot

Try out this version of the Hiro Wallet - download extension builds.

This PR refactors how we get the token metadata for post condition assets. Before, we would only get data and display it correctly if the wallet account had that particular token; this made swaps incorrect in the UI (see before image). In the example, I didn't have any USDA tokens so swapping for them rendered incorrectly in the post condition. Now, we use the token endpoint to show the metadata correctly (see after image). This issue was found investigating: Issue #1700

Issue #1745

Before:
Screen Shot 2021-09-28 at 1 11 13 PM

After:
Screen Shot 2021-09-28 at 3 57 19 PM

cc/ @aulneau @kyranjamie @fbwoolf

Testing:

  • Confirm all post conditions render correctly in the test-app.

@changeset-bot
Copy link

changeset-bot bot commented Sep 28, 2021 •
edited

🦋 Changeset detected

Latest commit: 64cbc09

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@stacks/wallet-web Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Sep 28, 2021 •
edited

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/blockstack/stacks-wallet-web/Bhy4rFUamdCTa8MsuhgGKYgEe5NR
✅ Preview: https://stacks-wallet-web-git-refactor-post-condition-assets-blockstack.vercel.app

@fbwoolf fbwoolf linked an issue Sep 28, 2021 that may be closed by this pull request
Post conditions token metadata even if user doesn't have token in their wallet #1745
Closed
@vercel vercel bot temporarily deployed to Preview September 28, 2021 21:14 Inactive
aulneau
aulneau approved these changes Sep 28, 2021
View reviewed changes
Copy link
Contributor

@aulneau aulneau left a comment

Choose a reason for hiding this comment

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

looks good! just a couple comments

src/store/transactions/post-conditions.hooks.ts Outdated Show resolved Hide resolved
src/store/transactions/post-conditions.hooks.ts Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview September 29, 2021 14:05 Inactive
@vercel vercel bot temporarily deployed to Preview September 29, 2021 15:51 Inactive
@fbwoolf fbwoolf force-pushed the refactor/post-condition-assets branch from 9b8c986 to 38222db Compare September 29, 2021 17:27
@vercel vercel bot temporarily deployed to Preview September 29, 2021 17:27 Inactive
@fbwoolf fbwoolf force-pushed the refactor/post-condition-assets branch from 38222db to 3a841bf Compare September 29, 2021 18:00
@vercel vercel bot temporarily deployed to Preview September 29, 2021 18:00 Inactive
kyranjamie
kyranjamie reviewed Sep 30, 2021
View reviewed changes
Copy link
Collaborator

@kyranjamie kyranjamie left a comment

Choose a reason for hiding this comment

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

Nice refactor @fbwoolf— most questions pertain to the exact function of useAssetFromPostCondition

src/store/assets/fungile-tokens.hooks.ts Outdated Show resolved Hide resolved
src/store/assets/fungible-tokens.ts Show resolved Hide resolved
src/store/transactions/post-conditions.hooks.ts Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview September 30, 2021 19:26 Inactive
kyranjamie
kyranjamie approved these changes Oct 1, 2021
View reviewed changes
Copy link
Collaborator

@kyranjamie kyranjamie left a comment

Choose a reason for hiding this comment

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

Looks good @fbwoolf

Small nit, let's rebase away the chore: add changeset commit, as this doesn't add anything valuable to the history

src/common/types.ts Show resolved Hide resolved
src/pages/transaction-signing/components/post-conditions/non-fungible-post-condition.tsx Outdated Show resolved Hide resolved
@fbwoolf fbwoolf force-pushed the refactor/post-condition-assets branch from 9550c6e to 64cbc09 Compare October 1, 2021 13:38
@vercel vercel bot temporarily deployed to Preview October 1, 2021 13:38 Inactive
@fbwoolf fbwoolf merged commit 0471c14 into main Oct 1, 2021
@fbwoolf fbwoolf deleted the refactor/post-condition-assets branch October 1, 2021 15:38
@blockstack-devops blockstack-devops mentioned this pull request Oct 1, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aulneau aulneau aulneau approved these changes

@kyranjamie kyranjamie kyranjamie approved these changes

@beguene beguene Awaiting requested review from beguene

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Post conditions token metadata even if user doesn't have token in their wallet
3 participants
@fbwoolf @kyranjamie @aulneau