feat(rename): Rename Nodes

[Jem256]

Closes #388

Description

Adds a rename node feature that allows users to rename a node.

Screenshots

[Jem256]

@jamaljsr here is the code

Edit: the last commit fixed the issue of the ports undefined error. Although the visual connector of the node to the backend has disappeared as shown below.

[jamaljsr]

The code currently on Github is missing the lightning.renameNode() function and is throwing an error, so I don't see the issue you're having.

[Jem256]

The code currently on Github is missing the lightning.renameNode() function and is throwing an error, so I don't see the issue you're having.

I've fixed all the issues. I kindly request for a review of my PR

[Jem256]

Thanks so much for working on this. When testing it, I found a few functional issues. polar-node-rename.mp4

1. When the network is started and I enter a new name then click Save, the network is stopped, but the node isn't renamed. I have to click Save a second time to rename the node.

2. After the rename, the node's open channels are no longer displayed.

3. The file paths to the cert & macaroons are still showing the old name. We should rename this folder as well.

4. I think it would be better to use a modal to have the user type the new name in so that we can access this from the right-click menu on each node.

5. Rename is not available for the Bitcoin Core and TAP nodes. Shouldn't this be supported for all node types?

Thank so much for the review. I am going to address all these. I have a question though, about Number 4, should we remove the button and simply have the modal accessible through the right click menu or should we have both?

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 100.00%. Comparing base (00197b7) to head (027079e).

Additional details and impacted files

@@            Coverage Diff             @@
##            master      #841    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files          141       143     +2     
  Lines         4663      4810   +147     
  Branches       903       933    +30     
==========================================
+ Hits          4663      4810   +147     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[jamaljsr]

I have a question though, about Number 4, should we remove the button and simply have the modal accessible through the right click menu or should we have both?

To be consistent with the other modals, I think you should remove the sidebar dropdown menu and add a new "Rename Node" button between the "Restart" and "Advanced Options" buttons in the Actions tab.

[Jem256]

Hey @jamaljsr do you have an idea why this is happening? when the app is restarted, the paths update to the new node name but the node also shows this error.

[jamaljsr]

Hey @Jem256, for each node, there is a folder on the host machine that contains the node's data. It's located at ~/.polar/networks/<network-id>/volumes/<implementation>/<node-name>/. It looks like Polar isn't renaming this dir when the node is renamed.

[Jem256]

~/.polar/networks//volumes///

how can I access this dir through the code to update the name? I can't seem to figure that out

[jamaljsr]

The DockerService.ensureDirs is currently the method that initially creates this dir when it starts the containers. This seems like the most appropriate place to rename the dir when the node is renamed. You can call it from the stores using injections.dockerService.

[jamaljsr]

Hey @Jem256 please let me know when you feel this is ready for review and I'll take a look at it.

[Jem256]

Hey @Jem256 please let me know when you feel this is ready for review and I'll take a look at it.

Hey Jamal, It should be ready sometime this week. Finishing up a few things today. I'll ping you when it's ready

[Jem256]

@jamaljsr please take a look and let me know what you think. The channel links are not yet working but I've tried to make a lot of other things work and would like to know your thoughts on my approach. The other thing I would like to highlight is that in case someone tries to rename the bitcoin nodes, the path to the node can't be updated because it doesn't have that property. Also on the issue of links, syncChart doesn't work in updating them. But the functions updateBackendLink and updateTapBackendLink worked for those corresponding links. Do you think we have to write a separate function like updateChannelLink for the channels

[Jem256]

Thanks for the updates. I've confirmed that all of the prior issues have been resolved.

I did discover one more problem which I described below.

The new code also needs unit test coverage. Are you able to work on that?

Thanks for the review. I'll work on the stated problem today. I also want to work on the unit tests over the weekend. However, is there a way to test coverage locally? Or for this particular branch?

[jamaljsr]

Yes, you can see coverage locally by running yarn test:ci. The output will show you which files have uncovered lines. Afterwards, you can also open the coverage/lcov-report/index.html file in a browser to view the same results in a nicer html format.

[jamaljsr]

@Jem256 Thanks so much for getting this new feature 99% done. I'm planning a new release in the coming days and wanted to make sure this gets in it. So I've rebased this branch and added a commit with unit tests bring the coverage back to 100%. I also found a few edge cases that needed fixing. I am going to merge this once the CI passes. Thank you again for the contribution.

[Jem256]

@jamaljsr thank you so much for this and for your patience and support to getting this feature this far.