Feat/taro-update-backend #669

amovfx · 2023-02-11T05:55:15Z

Create a draft of this for you to inspect workflow. I'm still working on tests and coverage. Also, compatibility filtering isn't working. I will figure that out.

Description

Modal for changeing the backend of taro nodes.

Steps to Test

Create 2 LND nodes and 1 Taro Node
Click the link between the taro node and lnd node
Click 'Change Backend' on info panel.
Change the lnd node selection.
Click 'Change Backend' button.

amovfx · 2023-02-12T20:32:27Z

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

+      if (compatibility) {
+        const requiredVersion = compatibility[lndBackend.version];
+        //version compatibility function breaks down
+        if (!requiredVersion) {


I had a lot of trouble hitting this if statement, I couldn't figure out how what test you were running where your coverage would clear.

amovfx

This has been rebased to the latest add-taro.

100% Coverage and linting checked.

What is the process to remove the conflicts with the main branch? Is that something that I do, or would you do?
Also, should I rename any variable named lndName to something like LNDname? I think I remember a note that said something along those lines?

jamaljsr · 2023-02-13T18:44:47Z

This needs a rebase on master since #656 and #667 were merged. One tip to make rebases a little easier is to do an interactive rebase then drop all of the commits that are already in the base branch.

git rebase -i feat/add-taro

I did a pass on this to test the UX. I noticed some things to improve:

You should be able to drag from the bottom port of a taro node to the top port of an LND node to open the change backend modal.
You shouldn't be able to change the backend if the taro node has assets. Those assets are embedded in UTXOs in the wallet of the LND node it is currently connected to. If you change the backend, then the taro node will no longer have access to the UTXOs in the LND wallet so those assets are essentially burned. The challenge is going to be how to tell if the taro node has assets if changing the backend when the network is stopped. There may be some data on disk that you can check. I'm not sure. This would require a bunch of testing scenarios and/or asking questions in the LND slack #taro channel about what users should & shouldn't do when changing nodes.

amovfx

I think this might be the last feature on the addTaro check list.

Coverage, testing and linting 100%.

src/components/designer/NetworkDesigner.spec.tsx

amovfx · 2023-02-15T17:11:13Z

I added the drag link feature.

You shouldn't be able to change the backend if the taro node has assets. Those assets are embedded in UTXOs in the wallet of the LND node it is currently connected to. If you change the backend, then the taro node will no longer have access to the UTXOs in the LND wallet so those assets are essentially burned. The challenge is going to be how to tell if the taro node has assets if changing the backend when the network is stopped. There may be some data on disk that you can check. I'm not sure. This would require a bunch of testing scenarios and/or asking questions in the LND slack #taro channel about what users should & shouldn't do when changing nodes.

Thanks for clarifying this behavior. I'll look into a solution. Sounds like a good challenge.

Proper casing on the usePrefixedTranslation

Added updateTaroBackendNode and updateTaroBackendLink

Added tarp send asset functionality.

Rebase onto latest add-taro

When dragging from LND network node to a Taro node or vice versa, the Change Taro Backend Modal will appear.

If a node as assets, disable the change taro backend button.

Taro change backend button is only enabled when the network is started and has no assets.

Changing taro backend is only available before the network starts and while a admin.macaron exists. If the macaron exists, and error pops up.

Cleaning up cruft

amovfx · 2023-02-18T03:11:44Z

K this is ready for review.

jamaljsr

Did another pass of the UX and found some issues. Can you resolve these before I dive into code review?

With carol-taro connected to carol, drag a new link between the two and click Change Backend. It will display an already connected error, which is correct. But now if you click on the link, the sidebar says invalid selection. You will also see a link remains if you drag from alice-taro to bob then hit Cancel in the modal. You have to remove the temporary link that gets added when dragging.
I am allowed to change the backend of a started network by dragging a link between the nodes. It should display an error
With the network started, the Change Taro backend modal displays a message stating the node will be restarted automatically. This msg should be removed since we won't allow changing after initial startup. Also remove the code that restarts the node
I'm getting failing tests in LinkDetails.spec.tsx due to this error
```
/Users/jamal/dev/polar/src/utils/config.ts:208
  join)(network.path, 'volumes',
                ^

TypeError: Cannot read properties of undefined (reading 'path')
```
Note: to make your life easier, it would be fine to ignore the unit tests for now. When the UX/code are approved and final, then you can write all of the test. I usually implement the full feature first, then write tests at the end, so I don't have to repeated rewrite the tests over and over again while iterating on the code.

amovfx · 2023-02-18T18:43:28Z

Thanks for the feedback and guidance, I'll get on this. If you are happy with this, Ill move on to cleaning up the tests.

Links are removed when modal is hidden. Restarting is removed. Change Taro Backend button errors after the network has been started.

amovfx · 2023-02-18T23:42:11Z

I hit these notes, thanks for running through this, this has been a challenging task.

jamaljsr

Functionality looks good now.

Did a first pass of code review.

src/components/common/form/LightningNodeSelect.spec.tsx

src/components/common/form/LightningNodeSelect.tsx

src/components/common/form/TaroNodeSelect.spec.tsx

src/components/designer/link/Backend.spec.tsx

src/components/designer/link/ChangeTaroBackendButton.tsx

src/store/models/modals.ts

src/store/models/network.spec.ts

src/store/models/network.ts

src/utils/tests/helpers.ts

amovfx · 2023-02-19T21:41:36Z

This should be good for review.

jamaljsr

Apologies for taking a while to review this. I missed your comment about it being ready. I just happened to check today because I was wondering about the status of this PR. In the future, it would be helpful if you can click on the re-request review icon on the PR. I typically check my review queue on a daily basis and it will stay on my list until I review it.

Thanks for the updates. After another pass on the code, there's just a few minor tweaks I'd suggest. Also there were some comments from my prior review that were not addressed. They are the ones not marked as unresolved.

jamaljsr · 2023-02-23T01:33:51Z

src/components/designer/link/ChangeTaroBackendButton.tsx

+      const taroNode = network.nodes.taro.find(n => n.name === taroName) as TarodNode;
+      const result = await exists(taroNode.paths.adminMacaroon);
+      setHasMacaroon(result);


You can run this code in the handleChangeClick callback instead of adding a state var.

src/i18n/locales/en-US.json

src/store/models/modals.ts

src/store/models/network.spec.ts

src/utils/tests/helpers.ts

Tests, lint and coverage 100%

amovfx · 2023-02-23T21:10:27Z

Thanks for pointing me to to the re-request review button. I'll be sure to use that in the future.
A couple messages aren't coming through.
In regard to the ChangeTaroBackendButton.btnText and .error. I am using them in the component.

jamaljsr

Thanks for the updates. I just reiterated a couple comments on the en-US.json file. Then this should be good to go.

src/i18n/locales/en-US.json

amovfx · 2023-02-24T21:50:12Z

Good to go.

jamaljsr

Perfect 👌

Thanks for iterating on this. Going to squash merge this because there are a lot of commits and the diff is not that big.

amovfx commented Feb 12, 2023

View reviewed changes

amovfx marked this pull request as ready for review February 12, 2023 21:33

amovfx commented Feb 14, 2023

View reviewed changes

src/components/designer/NetworkDesigner.spec.tsx Outdated Show resolved Hide resolved

Andrew Oseen added 24 commits February 15, 2023 09:23

feat(taro-update-backend-modal): update modal

af18b1e

fix(taro-backend-link): review notes

9e53229

fix(caseing): tarobackend

62a558e

Proper casing on the usePrefixedTranslation

feat(taro-update-backend-modal): update modal

dee58cf

fix(change-taro-backend-modal): actions and thunks

c0fa2bc

Added updateTaroBackendNode and updateTaroBackendLink

fix(tests): changetarobackendmodal tests

f6b4b04

fix(change-taro-backend-modal): error messages

d0d8c12

fix(change-taro-backend-modal): restart message

5812f1a

fix(change-taro-backend-modal): compatibility alert

e7d444d

fix(tests): added more tests for coverage

8a44cf2

fix(coverage): increase coverage

c2a9e07

fix(coverage): on changetarobackendmodal

9028dda

fix(taro-backend-link): review notes

7964b15

fix(bad-merge): missed translation and ChangeTaroBackendButton

90dee8f

fix(typo barf): bad text made it into jest descriptor

0f50434

fix(compatibility error clear): test for error clear

c6e6ff4

fix(rename): lndName to LNDName

5e70d01

fix(taro-backend-link): review notes

022da34

fix(squash commit): send asset modal

ad43d30

Added tarp send asset functionality.

feat(taro-update-backend-modal): update modal

1e23e97

Rebase onto latest add-taro

fix(bad-merge): added missing import for ChangeTaroBackendModel

5b05512

fix(broken test): question for jamal

141b80c

fix(test): change taro backend modal passes

8fe8756

feat(drag-taro-link): change taro backed modal

387e943

When dragging from LND network node to a Taro node or vice versa, the Change Taro Backend Modal will appear.

amovfx force-pushed the feat/taro-update-backend branch from 32f8991 to 387e943 Compare February 15, 2023 17:36

Andrew Oseen added 5 commits February 15, 2023 12:11

fix(change-taro-backend-button): disabled state

b1e2e9c

If a node as assets, disable the change taro backend button.

fix(taro-backend-button): disable

99ab58d

Taro change backend button is only enabled when the network is started and has no assets.

fix(change-taro-backend): workflow

9450d40

Changing taro backend is only available before the network starts and while a admin.macaron exists. If the macaron exists, and error pops up.

fix(taro-taro-backend): ci and coverage

922684a

fix(review): small corrections

e4ccad4

Cleaning up cruft

jamaljsr reviewed Feb 18, 2023

View reviewed changes

fix(links): properly remove links

a406911

Links are removed when modal is hidden. Restarting is removed. Change Taro Backend button errors after the network has been started.

jamaljsr reviewed Feb 19, 2023

View reviewed changes

Andrew Oseen added 3 commits February 19, 2023 12:22

fix(review): first pass review

cebae46

fix(review notes): more fixes

7ba180e

fix(change-taro-backend-button): replaced then with await

740f34a

fix(tests): repair tests

2fd3fcf

jamaljsr reviewed Feb 23, 2023

View reviewed changes

fix(review-notes): latest review notes

e207479

Tests, lint and coverage 100%

amovfx requested a review from jamaljsr February 23, 2023 21:08

jamaljsr reviewed Feb 24, 2023

View reviewed changes

src/i18n/locales/en-US.json Outdated Show resolved Hide resolved

src/i18n/locales/en-US.json Outdated Show resolved Hide resolved

fix(en-US.json): remove unused keys

19380da

amovfx requested a review from jamaljsr February 24, 2023 21:50

jamaljsr approved these changes Feb 25, 2023

View reviewed changes

jamaljsr merged commit e030170 into jamaljsr:feat/add-taro Feb 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/taro-update-backend #669

Feat/taro-update-backend #669

amovfx commented Feb 11, 2023 •

edited

Loading

amovfx Feb 12, 2023

amovfx left a comment •

edited

Loading

jamaljsr commented Feb 13, 2023

amovfx left a comment

amovfx commented Feb 15, 2023

amovfx commented Feb 18, 2023

jamaljsr left a comment

amovfx commented Feb 18, 2023 •

edited

Loading

amovfx commented Feb 18, 2023

jamaljsr left a comment

amovfx commented Feb 19, 2023

jamaljsr left a comment

jamaljsr Feb 23, 2023

amovfx commented Feb 23, 2023

jamaljsr left a comment

amovfx commented Feb 24, 2023

jamaljsr left a comment

Feat/taro-update-backend #669

Feat/taro-update-backend #669

Conversation

amovfx commented Feb 11, 2023 • edited Loading

Description

Steps to Test

amovfx Feb 12, 2023

Choose a reason for hiding this comment

amovfx left a comment • edited Loading

Choose a reason for hiding this comment

jamaljsr commented Feb 13, 2023

amovfx left a comment

Choose a reason for hiding this comment

amovfx commented Feb 15, 2023

amovfx commented Feb 18, 2023

jamaljsr left a comment

Choose a reason for hiding this comment

amovfx commented Feb 18, 2023 • edited Loading

amovfx commented Feb 18, 2023

jamaljsr left a comment

Choose a reason for hiding this comment

amovfx commented Feb 19, 2023

jamaljsr left a comment

Choose a reason for hiding this comment

jamaljsr Feb 23, 2023

Choose a reason for hiding this comment

amovfx commented Feb 23, 2023

jamaljsr left a comment

Choose a reason for hiding this comment

amovfx commented Feb 24, 2023

jamaljsr left a comment

Choose a reason for hiding this comment

amovfx commented Feb 11, 2023 •

edited

Loading

amovfx left a comment •

edited

Loading

amovfx commented Feb 18, 2023 •

edited

Loading