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.

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

Add API functions to start and stop local Monero node #76

Merged
merged 2 commits into from
Apr 4, 2022

Conversation

duriancrepe
Copy link
Contributor

Tests the local Monero node functionality. The test first attempts to stop any instances of the local monero node and check for expected errors. If the local instance is an external process, then the test will print a warning that fully starting / stopping is not tested. Local instance is tested by providing custom MoneroNodeSettings and attempting to connect to it using monerojs.

Resolves haveno-dex/haveno#137

Copy link
Contributor

@woodser woodser left a comment

Choose a reason for hiding this comment

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

I'm getting an error in the test "Can manage Monero daemon connections" with these 2 PRs, traced from HavenoDaemon.test.ts:398:

 FAIL  src/HavenoDaemon.test.ts (57.514 s)
  ● Can manage Monero daemon connections

    assert.equal(received, expected)

    Expected value to be equal to:
      "http://localhost:58081"
    Received:
      "http://localhost:38081"

    Difference:

    - Expected
    + Received

    - http://localhost:58081
    + http://localhost:38081

      1705 |
      1706 | function testConnection(connection: UrlConnection, url?: string, onlineStatus?: OnlineStatus, authenticationStatus?: AuthenticationStatus, priority?: number) {
    > 1707 |   if (url) assert.equal(connection.getUrl(), url);
           |                   ^
      1708 |   assert.equal(connection.getPassword(), ""); // TODO (woodser): undefined instead of ""?
      1709 |   assert.equal(connection.getUsername(), "");
      1710 |   if (onlineStatus !== undefined) assert.equal(connection.getOnlineStatus(), onlineStatus);

      at testConnection (src/HavenoDaemon.test.ts:1707:19)
      at Object.<anonymous> (src/HavenoDaemon.test.ts:398:5)

src/HavenoDaemon.test.ts Outdated Show resolved Hide resolved
@duriancrepe
Copy link
Contributor Author

I'm getting an error in the test "Can manage Monero daemon connections" with these 2 PRs, traced from HavenoDaemon.test.ts:398:

 FAIL  src/HavenoDaemon.test.ts (57.514 s)
  ● Can manage Monero daemon connections

    assert.equal(received, expected)

    Expected value to be equal to:
      "http://localhost:58081"
    Received:
      "http://localhost:38081"

    Difference:

    - Expected
    + Received

    - http://localhost:58081
    + http://localhost:38081

      1705 |
      1706 | function testConnection(connection: UrlConnection, url?: string, onlineStatus?: OnlineStatus, authenticationStatus?: AuthenticationStatus, priority?: number) {
    > 1707 |   if (url) assert.equal(connection.getUrl(), url);
           |                   ^
      1708 |   assert.equal(connection.getPassword(), ""); // TODO (woodser): undefined instead of ""?
      1709 |   assert.equal(connection.getUsername(), "");
      1710 |   if (onlineStatus !== undefined) assert.equal(connection.getOnlineStatus(), onlineStatus);

      at testConnection (src/HavenoDaemon.test.ts:1707:19)
      at Object.<anonymous> (src/HavenoDaemon.test.ts:398:5)

I restored the previous behavior which prioritized connecting to the last known monero connection setting. If the connection failed, then attempt to start the local monero node if the setting exists.

Verified both Can manage Monero daemon connections and Can start and stop local Monero node pass.

I also wanted to run Can complete a trade to test the additional === operator cleanup change I made, however having issues mining blocks with the monero shared test network at the moment.

The interaction between connection manager and local node may need further refinement depending user experience, since if the local node is force killed or stuck on errors then it will fallback and reset to using the connection manager connections but that may not be desired behavior.

src/HavenoDaemon.test.ts Outdated Show resolved Hide resolved
src/HavenoDaemon.test.ts Outdated Show resolved Hide resolved
src/HavenoDaemon.test.ts Outdated Show resolved Hide resolved
@woodser
Copy link
Contributor

woodser commented Mar 28, 2022

If the connection failed, then attempt to start the local monero node if the setting exists.

I'm thinking it would be too automated for the backend to automatically start and connect to a local node if they last used a different connection which is offline and they previously started a local node. They may want to simply select a different node.

However, I think it would make sense to automatically start the local node on startup if they last used it and it's offline, and maybe this is the only change warranted to CoreMoneroConnectionsService (as a last check in initialize()).

The interaction between connection manager and local node may need further refinement depending user experience, since if the local node is force killed or stuck on errors then it will fallback and reset to using the connection manager connections but that may not be desired behavior.

The connection manager should be independent. If the local node goes offline, the connection manager already supports polling for this and an option to automatically switch to the next best available connection.

I hope this make sense. Please let me know if you think I'm missing something.

@duriancrepe
Copy link
Contributor Author

The connection manager should be independent. If the local node goes offline, the connection manager already supports polling for this and an option to automatically switch to the next best available connection.

I hope this make sense. Please let me know if you think I'm missing something.

Yes, this makes sense. I'll come up with a solution that is cleaner and less surprising for end user.

@woodser woodser merged commit ad26aae into haveno-dex:master Apr 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add API functions to start and stop local Monero node [$300]
2 participants