Feat/send-taro-asset #656

amovfx · 2023-01-14T00:43:28Z

SendAssetModal is blocked out.

Description

Rework from our previous conversations. I can't really see this being more than this, all that send asset needs is the taro address which seems a lot like an lnd invoice.

I want to put a menu on the bottom to help create a quick address like I did on the new address form. I'm thinking of the same idea, a collapsible one that allows you to select a taro node, asset, and amount to populate the address field.

This is branched from new-address-modal

amovfx · 2023-02-01T23:34:53Z

If you are interested in trying out the complete workflow, it is available. This is branched off of the latest new-address-modal branch in my repo

jamaljsr · 2023-02-02T14:26:31Z

Initial first pass on the UX.

If I paste an address then remove it, a toast error msg is displayed. It shouldn't attempt to decode an address if the value is blank. It should also hide the address info section when a decoding error occurs.
For invalid addresses, it would be better to display the validation error inline on the form instead of as a toast message so that it doesn't disappear after a few seconds.
I'm not really a fan of the look of the green alert showing the address info. It's great to display this info, but I'd remove the green background. Just display the info on the modal background.
We shouldn't display the "Deposit enough funds .." option if the node already has enough funds to send the asset. I just realized the Mint Asset form should be changed as well. I missed this in that PR review.

This PR can now be rebased to drop the duplicate commits.

amovfx · 2023-02-02T22:44:24Z

Awesome, thanks for this feed back. I was thinking the same thing about #4 today too.

jamaljsr · 2023-02-05T01:39:44Z

Thanks for the updates. This is much better.

Just a couple remaining suggestions:

Do not show the "Address Info" divider until the details are also displayed. It looks weird having the divider there with no content below it.
When there is a error decoding the address, display the error message returned from tarod instead of the generic "ERROR: Invalid Address". Also, you can make the message red via the validateStatus prop on the <Form.Item>.

Once these updates are made, this PR will be ready for code review. Please don't forget to rebase.

Added tarp send asset functionality.

Fixed test to check for label to deposit enough funds for mint asset modal

NewAddressResponse renamed to AddressResponse

amovfx · 2023-02-07T01:59:50Z

Hey, after the rebase I'm having trouble with the decode throwing an error. I will investigate and fix.

jamaljsr · 2023-02-07T02:58:44Z

The diff looks pretty good after the rebase. Let me know when you're ready for me to do another review.

amovfx · 2023-02-08T03:36:53Z

I think it was a bad refresh. I just took it for a spin and had successful sends.

jamaljsr

For the UX, the comments from my last review still remain. Also, i think it would be better to put "Send Asset" at the top of the context menu and Actions sidebar since I suspect this would be the most used action.

I did an initial pass over the code. Can you try to work on more tests to get coverage over all the new code paths. Just run yarn ci to see which lines in the code aren't covered. You can also open the generated ./coverage/lcov-report/index.html file in a browser to get a more detailed look at the coverage of each file.

src/components/designer/taro/actions/MintAssetModal.spec.tsx

src/components/designer/taro/actions/SendAssetModal.tsx

src/components/designer/taro/actions/SendAssetModal.spec.tsx

src/lib/taro/types.ts

src/shared/tarodTypes.ts

src/store/models/modals.ts

src/store/models/taro.ts

src/utils/tests/testStateDefaults.ts

Hit notes and made sure tests hit coverage.

Cleaned up the tests

jamaljsr

Functionality is still good but I have some suggestions for your updates.

Also, there's still linter warnings and code not covered by tests. Remember to run yarn ci to find these locally.

src/components/designer/taro/actions/SendAssetModal.spec.tsx

src/components/designer/taro/actions/MintAssetModal.tsx

src/utils/tests/testStateDefaults.ts

linting errors, missing parameter, and testing conventions

amovfx · 2023-02-11T03:03:29Z

Sorry for missing those linter errors. Is there away I can configure git to not submit unless the linter passes?
You use that husky thing, is it possible for me to enable it there some how?
Thanks for taking your time to review this.

jamaljsr · 2023-02-11T15:37:09Z

The current husky config only prevents committing when there are linter errors, not warnings. I'm fine with adding warnings since it'll make it easier to catch those as well. You can do it in this branch by changing the eslint command to eslint --ext .ts,.tsx --fix --max-warnings 0 in .litstagedrc.

The coverage is still not at 100 in all 4 columns. Are you still working on this? If you are unable to figure out how to test all code paths, please let me know which specific issues you're facing and I'll try to help.

jamaljsr · 2023-02-11T16:06:02Z

src/components/designer/NodeContextMenu.tsx

+    addItemIf(
+      'sendAsset',
+      <SendAssetButton type={'menu'} node={node as TaroNode} />,
+      isStarted && isTaro,
+    ),


I mentioned in the previous review: i think it would be better to put "Send Asset" at the top of the context menu and Actions sidebar since I suspect this would be the most used action.

jamaljsr · 2023-02-11T16:06:24Z

src/components/designer/taro/ActionsTab.tsx

@@ -24,6 +24,7 @@ const ActionsTab: React.FC<Props> = ({ node }) => {
        <>
          <MintAssetButton node={node} />
          <NewAddressButton node={node} />
+          <SendAssetButton node={node} />


Move this to the top of the list as well.

amovfx · 2023-02-11T22:47:45Z

I am actually struggling to hit some code paths. Sometimes it's with if statements, I looked it up on google, and it has something to do when an if is missing an else statement after. I'll give it another shot asap.

example:
Things like this

    if (lndNode) {
      getWalletBalance(lndNode);
      getAssets(thisTaroNode);
    }

This was the reason for the previous

lndNode && getWalletBalance(lndNode) && getAssets(thisTaroNode);

The above has better coverage results.
For the other ones, I learned about the network and designer tests in the last few days, so I think I can hit those code paths now.

Send asset is at the top now.

100% coverage

amovfx · 2023-02-12T05:34:18Z

This is ready to review. 100% code coverage. Excellent learning experience. Thanks for taking the time on this and for all the added tests you did in the previous commit to bring the coverage up. Moving forward Ill make sure all prs have 100% coverage.

jamaljsr

This looks good to go! Great work on the PR. Thanks for continuing to iterate on it.

amovfx force-pushed the feat/send-taro-asset branch from 56dbb36 to 35110f8 Compare February 1, 2023 21:14

amovfx force-pushed the feat/send-taro-asset branch from 887b71a to 62311d0 Compare February 2, 2023 16:35

amovfx marked this pull request as ready for review February 5, 2023 21:42

jamaljsr mentioned this pull request Feb 7, 2023

feat(tapd): Add support for Taproot Assets Protocol nodes #641

Merged

16 tasks

amovfx force-pushed the feat/send-taro-asset branch 2 times, most recently from 85b7d08 to ea48f71 Compare February 7, 2023 01:18

fix(squash commit): send asset modal

2ed4503

Added tarp send asset functionality.

amovfx force-pushed the feat/send-taro-asset branch from 9480c7e to 2ed4503 Compare February 7, 2023 01:23

Andrew Oseen added 2 commits February 6, 2023 17:40

fix(test): mint asset modal test

a61d648

Fixed test to check for label to deposit enough funds for mint asset modal

fix(missing type): fixed missed refactor

5c7b685

NewAddressResponse renamed to AddressResponse

amovfx closed this Feb 7, 2023

amovfx reopened this Feb 7, 2023

jamaljsr reviewed Feb 8, 2023

View reviewed changes

Andrew Oseen added 3 commits February 8, 2023 20:37

fix(send-asset-modal): review notes

d322a43

Hit notes and made sure tests hit coverage.

fix(refactor): test

6e522d4

Cleaned up the tests

fix(tests): refactor for readability

349d07c

jamaljsr reviewed Feb 10, 2023

View reviewed changes

fix(review notes): various

c4ac526

linting errors, missing parameter, and testing conventions

jamaljsr reviewed Feb 11, 2023

View reviewed changes

fix(button placement): missed review note

47adf30

Send asset is at the top now.

Andrew Oseen added 2 commits February 11, 2023 21:27

fix(coverage): improved tests and code

1741a2b

100% coverage

fix(typing): stronger typing to avoid conditional

9891b34

fix(linting): added --max-warnings 0

4491a62

jamaljsr approved these changes Feb 13, 2023

View reviewed changes

jamaljsr merged commit 5bb476a into jamaljsr:feat/add-taro Feb 13, 2023

jamaljsr mentioned this pull request Feb 13, 2023

Feat/taro-update-backend #669

Merged

Feat/send-taro-asset #656

Feat/send-taro-asset #656

Conversation

amovfx commented Jan 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

amovfx commented Feb 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamaljsr commented Feb 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amovfx commented Feb 2, 2023

Uh oh!

jamaljsr commented Feb 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amovfx commented Feb 7, 2023

Uh oh!

jamaljsr commented Feb 7, 2023

Uh oh!

amovfx commented Feb 8, 2023

Uh oh!

jamaljsr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jamaljsr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amovfx commented Feb 11, 2023

Uh oh!

jamaljsr commented Feb 11, 2023

Uh oh!

jamaljsr Feb 11, 2023

Choose a reason for hiding this comment

Uh oh!

jamaljsr Feb 11, 2023

Choose a reason for hiding this comment

Uh oh!

amovfx commented Feb 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amovfx commented Feb 12, 2023

Uh oh!

jamaljsr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amovfx commented Jan 14, 2023 •

edited

Loading

amovfx commented Feb 1, 2023 •

edited

Loading

jamaljsr commented Feb 2, 2023 •

edited

Loading

jamaljsr commented Feb 5, 2023 •

edited

Loading

amovfx commented Feb 11, 2023 •

edited

Loading