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

[jamaljsr]

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?

[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 ✅

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     

☔ View full report in Codecov by Sentry.
📢 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

[jamaljsr]

Thanks so much for working on this feature. It has been requested many times in the past. You are off to a great start. I've left a bunch of feedback on where I see room for improvement.

One strange issue I noticed when testing was that I couldn't select the text in the input field of the modal. Instead, it was dragging the node on the canvas underneath the modal. I'm not sure how that's even possible but my hunch is it's related to where you have the Modal component being mounted. I mentioned some details about this in a comment below.

polar-rename.mp4

[jamaljsr]

This is looking amazing! Great work 👍

I just have a bit more feedback on the latest changes.

Also, I noticed this error when trying to rename a Core Lightning node. Does it work fine for you?

[jamaljsr]

Thanks so much for the additional updates. This is looking really good. I found a few remaining issues, but this should be complete once these are fixed.

The last things remaining would be to rebase this branch on master and add/update the unit tests to fix the failures and get the coverage back up. Testing all of the code paths can be pretty tricky sometimes so please let me know if you need any assistance with this.

[jamaljsr]

I noticed that we don't have any validation here, so entering weird characters like "alice : / ` asd" puts the network in a totally broken state. You cannot even delete the network after doing that.

Here's how to restrict the input to letters, numbers, -, and _. This

        <Form.Item
          name="newNodeName"
          label={l('label')}
          rules={[
            { required: true, message: l('cmps.forms.required') },
            { pattern: /^[a-zA-Z0-9_-]+$/, message: l('invalidName') },
          ]}
        >

This invalidName string in en-US.json can be "Can only contain letters, numbers, underscores, and dashes.."

[jamaljsr]

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?

[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.