Conversation
Signed-off-by: emiliyank <[email protected]>
Signed-off-by: emiliyank <[email protected]>
Signed-off-by: emiliyank <[email protected]>
Signed-off-by: emiliyank <[email protected]>
Signed-off-by: emiliyank <[email protected]>
…ACCOUNT Signed-off-by: emiliyank <[email protected]>
Signed-off-by: emiliyank <[email protected]>
…ccount-id' into feature/hip-1299-changing-node-account-id
Signed-off-by: emiliyank <[email protected]>
Signed-off-by: emiliyank <[email protected]>
Signed-off-by: emiliyank <[email protected]>
Signed-off-by: emiliyank <[email protected]>
Signed-off-by: emiliyank <[email protected]>
Signed-off-by: emiliyank <[email protected]>
…ACCOUNT Signed-off-by: emiliyank <[email protected]>
Signed-off-by: emiliyank <[email protected]>
Signed-off-by: emiliyank <[email protected]>
Signed-off-by: emiliyank <[email protected]>
…ccount-id' into feature/hip-1299-changing-node-account-id
Signed-off-by: emiliyank <[email protected]>
Signed-off-by: emiliyank <[email protected]>
Signed-off-by: emiliyank <[email protected]>
Signed-off-by: emiliyank <[email protected]>
Signed-off-by: emiliyank <[email protected]>
Signed-off-by: emiliyank <[email protected]>
WalkthroughAdds Client.updateNetworkFromAddressBook(); changes Executable retry/control flow to treat INVALID_NODE_ACCOUNT as SERVER_ERROR, advance to the next node, increase offending-node backoff, and schedule an address-book update; updates CI Hiero action and Gradle invocations; adds unit and extensive integration tests for node updates. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Executable
participant Node
participant Network
Client->>Executable: execute(request)
Executable->>Node: send gRPC request
alt Node returns INVALID_NODE_ACCOUNT
Node-->>Executable: INVALID_NODE_ACCOUNT
Executable->>Executable: map to SERVER_ERROR
Executable->>Executable: advanceRequest() %% switch to next node
Executable->>Network: increaseBackoff(offendingNode)
par async address-book update
Executable->>Client: schedule updateNetworkFromAddressBook()
Client->>Client: perform AddressBookQuery
Client->>Network: setNetworkFromAddressBook(...)
and immediate retry on next node
Executable->>Node: retry request on next node
end
else Node returns success/other
Node-->>Executable: response / status
Executable->>Client: handleResponse(..., Client) and return result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
sdk/src/main/java/com/hedera/hashgraph/sdk/Client.java (1)
1433-1436: Consider propagatingInterruptedExceptionproperly.When catching
Exceptionbroadly, if anInterruptedExceptionoccurs (which can be wrapped inTimeoutExceptionor thrown directly), the thread's interrupted status should be restored:} catch (TimeoutException e) { logger.warn("Failed to fetch address book: {}", e.getMessage()); + } catch (InterruptedException e) { + logger.warn("Address book update interrupted: {}", e.getMessage()); + Thread.currentThread().interrupt(); } catch (Exception e) { logger.warn("Failed to update address book", e); }sdk/src/testIntegration/java/com/hedera/hashgraph/sdk/test/integration/NodeUpdateTransactionIntegrationTest.java (3)
4-8: Remove redundant static imports.Lines 5-6 are redundant since line 4 already imports all static members from
Assertions. Also, consider using explicit imports instead of the wildcard on line 8 for better readability.-import static org.assertj.core.api.Assertions.*; -import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatThrownBy;
199-201: Consider using a polling mechanism instead of fixed sleep.The 10-second
Thread.sleep()for mirror node synchronization is brittle. Consider using a polling/retry mechanism with a shorter interval that checks if the data is available, up to a maximum timeout. This would make tests faster when the mirror node syncs quickly and more reliable when it takes longer.Example approach:
// Poll for mirror node sync with exponential backoff int maxAttempts = 20; for (int i = 0; i < maxAttempts; i++) { try { // Try operation that depends on mirror node data break; } catch (Exception e) { if (i == maxAttempts - 1) throw e; Thread.sleep(500 * (1 << Math.min(i, 4))); // 500ms, 1s, 2s, 4s, 8s... } }
68-125: Document why this test is disabled.The
@Disabledannotation should include a reason explaining why the test is disabled and what needs to be done to enable it. This helps future maintainers understand the test's status.@Test -@Disabled +@Disabled("Requires proper node admin key setup - see HIP-1299 implementation") @DisplayName(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/build.yml(3 hunks)sdk/src/main/java/com/hedera/hashgraph/sdk/Client.java(1 hunks)sdk/src/main/java/com/hedera/hashgraph/sdk/Executable.java(12 hunks)sdk/src/test/java/com/hedera/hashgraph/sdk/ExecutableTest.java(3 hunks)sdk/src/testIntegration/java/com/hedera/hashgraph/sdk/test/integration/NodeUpdateTransactionIntegrationTest.java(2 hunks)
🔇 Additional comments (9)
.github/workflows/build.yml (1)
202-203: LGTM - Test configuration flags added appropriately.The
--rerun-tasksflag ensures consistent test execution, and-PskipNodeUpdateTest=truecorrectly excludes node update tests from the standard test job since they require the DAB (Dynamic Address Book) environment tested separately.sdk/src/test/java/com/hedera/hashgraph/sdk/ExecutableTest.java (2)
533-562: LGTM - Good coverage for INVALID_NODE_ACCOUNT handling.The test properly validates that:
INVALID_NODE_ACCOUNTtriggers retry behavior across multiple nodesnetwork.increaseBackoff()is called to mark nodes as unhealthy- Eventually throws
MaxAttemptsExceededExceptionwhen all nodes fail
564-592: Test correctly documents async limitations.The comment on lines 590-591 appropriately acknowledges that the async address book update cannot be easily verified in this unit test context. The integration tests provide that coverage.
sdk/src/testIntegration/java/com/hedera/hashgraph/sdk/test/integration/NodeUpdateTransactionIntegrationTest.java (1)
527-624: LGTM - Comprehensive integration test for network configuration updates.This test effectively validates the end-to-end flow:
- Initial network configuration verification
- Node account ID update via
NodeUpdateTransaction- SDK handling of
INVALID_NODE_ACCOUNTwith retry- Verification that subsequent transactions work with the new node account ID
- Proper cleanup by reverting the node account ID
sdk/src/main/java/com/hedera/hashgraph/sdk/Executable.java (5)
446-455: LGTM - Consistent INVALID_NODE_ACCOUNT handling.The implementation correctly:
- Advances to the next node first
- Logs the issue at TRACE level
- Triggers address book update for network refresh
- Marks the node as unhealthy via
increaseBackoff()This aligns with the Go SDK's behavior for
executionStateRetryWithAnotherNode.
792-803: Consider async address book update to avoid blocking.In the async execution path,
client.updateNetworkFromAddressBook()is a synchronous call that performs network I/O. While the method internally catches exceptions, it could still block the async flow briefly.Since the method already logs and swallows errors, this is acceptable for now, but for improved async purity, consider adding an async variant (
updateNetworkFromAddressBookAsync()) in the future.
903-906: LGTM - Correct execution state mapping.Mapping
INVALID_NODE_ACCOUNTtoSERVER_ERRORensures immediate retry with the next node without delay, which is the correct behavior when a node's account ID has changed.
1011-1016: LGTM - Correctly skips backoff decrease for invalid nodes.Not calling
decreaseBackoff()forINVALID_NODE_ACCOUNTis correct because:
- The node should remain marked as unhealthy
- The actual backoff increase happens after
advanceRequest()in the calling codeThis matches the comment explaining that INVALID_NODE_ACCOUNT handling happens after advancement.
698-702: Good documentation of the design change.The comment clearly explains that node advancement now happens explicitly in retry logic after error detection, rather than automatically in
getRequestForExecute(). This helps future maintainers understand the flow.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
sdk/src/testIntegration/java/com/hedera/hashgraph/sdk/test/integration/NodeUpdateTransactionIntegrationTest.java (3)
4-8: Remove redundant explicit imports.The wildcard import on line 4 (
import static org.assertj.core.api.Assertions.*;) already includesassertThatandassertThatThrownBy, making lines 5-6 redundant.Similarly, line 8 uses a wildcard SDK import which is generally discouraged as it can lead to namespace pollution and makes it harder to track dependencies.
-import static org.assertj.core.api.Assertions.*; -import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatThrownBy; -import com.hedera.hashgraph.sdk.*; +import com.hedera.hashgraph.sdk.AccountCreateTransaction; +import com.hedera.hashgraph.sdk.AccountDeleteTransaction; +import com.hedera.hashgraph.sdk.AccountId; +import com.hedera.hashgraph.sdk.Client; +import com.hedera.hashgraph.sdk.Endpoint; +import com.hedera.hashgraph.sdk.Hbar; +import com.hedera.hashgraph.sdk.Key; +import com.hedera.hashgraph.sdk.NodeUpdateTransaction; +import com.hedera.hashgraph.sdk.PrivateKey; +import com.hedera.hashgraph.sdk.ReceiptStatusException; +import com.hedera.hashgraph.sdk.Status;
199-200: Consider replacing fixedThread.sleepwith polling.Fixed 10-second sleeps (also at lines 486 and 547) waiting for mirror node synchronization can cause flaky tests or unnecessary delays. Consider implementing a polling mechanism that checks for the expected state with a timeout.
For example, a helper method that polls until the address book reflects the change:
private void waitForMirrorNodeSync(Client client, AccountId expectedNodeAccountId, Duration timeout) throws Exception { var deadline = Instant.now().plus(timeout); while (Instant.now().isBefore(deadline)) { try { client.updateNetworkFromAddressBook(); if (client.getNetwork().containsValue(expectedNodeAccountId)) { return; } } catch (Exception ignored) {} Thread.sleep(1000); } throw new TimeoutException("Mirror node did not sync within timeout"); }
70-74: Add explanation for why test is disabled.The test
shouldSucceedWhenUpdatingNodeAccountIdWithProperSignaturesis marked@Disabledwithout any explanation (unlike line 346-348 which has a TODO comment). Consider adding a comment explaining why this test is disabled and what needs to change for it to be enabled.@Test @Disabled +// TODO - Test requires node admin key to be available for signing. Currently blocked on local network setup. @DisplayName( "Given a node with an existing account ID, when a NodeUpdateTransaction is submitted to change the account ID to a new valid account with signatures from both the node admin key and the account ID key, then the transaction succeeds")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
sdk/src/main/java/com/hedera/hashgraph/sdk/Client.java(1 hunks)sdk/src/testIntegration/java/com/hedera/hashgraph/sdk/test/integration/NodeUpdateTransactionIntegrationTest.java(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
sdk/src/testIntegration/java/com/hedera/hashgraph/sdk/test/integration/NodeUpdateTransactionIntegrationTest.java (2)
sdk/src/main/java/com/hedera/hashgraph/sdk/Client.java (1)
Client(39-1677)sdk/src/main/java/com/hedera/hashgraph/sdk/PrivateKey.java (1)
PrivateKey(20-460)
sdk/src/main/java/com/hedera/hashgraph/sdk/Client.java (1)
sdk/src/main/java/com/hedera/hashgraph/sdk/FileId.java (1)
FileId(14-304)
🔇 Additional comments (3)
sdk/src/testIntegration/java/com/hedera/hashgraph/sdk/test/integration/NodeUpdateTransactionIntegrationTest.java (3)
159-241: Good coverage of INVALID_NODE_ACCOUNT retry flow.This test comprehensively validates the SDK's ability to detect
INVALID_NODE_ACCOUNTerrors, retry with alternate nodes, and successfully complete transactions. The cleanup at the end to revert the node account ID helps ensure test isolation.
568-602: Good use of helper methods to reduce test duplication.The extracted helper methods (
createAccount,updateNodeAccountId,executeAccountCreate) effectively reduce code duplication across test methods. This improves maintainability and makes the test intent clearer.
527-566: Validates address book update integration correctly.This test verifies the critical behavior that after encountering
INVALID_NODE_ACCOUNT, the SDK updates its network configuration with the latest node account IDs. The assertion at lines 556-561 that checksclient.getNetwork()contains the new account ID directly validates theupdateNetworkFromAddressBook()integration.
| /** | ||
| * Trigger an immediate address book update to refresh the client's network with the latest node information. | ||
| * This is useful when encountering INVALID_NODE_ACCOUNT_ID errors to ensure subsequent transactions | ||
| * use the correct node account IDs. | ||
| * | ||
| * @return {@code this} | ||
| */ | ||
| public synchronized Client updateNetworkFromAddressBook() { | ||
| try { | ||
| var fileId = FileId.getAddressBookFileIdFor(this.shard, this.realm); | ||
|
|
||
| logger.debug("Fetching address book from file {}", fileId); | ||
|
|
||
| // Execute synchronously - no async complexity | ||
| var addressBook = new AddressBookQuery().setFileId(fileId).execute(this); // ← Synchronous! | ||
|
|
||
| logger.debug("Received address book with {} nodes", addressBook.nodeAddresses.size()); | ||
|
|
||
| // Update the network | ||
| this.setNetworkFromAddressBook(addressBook); | ||
|
|
||
| logger.info("Address book update completed successfully"); | ||
|
|
||
| } catch (TimeoutException e) { | ||
| logger.warn("Failed to fetch address book: {}", e.getMessage()); | ||
| } catch (Exception e) { | ||
| logger.warn("Failed to update address book", e); | ||
| } | ||
|
|
||
| return this; | ||
| } |
There was a problem hiding this comment.
InterruptedException handling may lose thread interrupt status.
The setNetworkFromAddressBook method declares throws InterruptedException, but when caught by the generic Exception catch block at line 1432, the thread's interrupt status is not restored. This can cause issues in concurrent environments where interrupt propagation is expected.
Consider either:
- Catching
InterruptedExceptionexplicitly and restoring the interrupt status - Or re-throwing as an unchecked exception
} catch (TimeoutException e) {
logger.warn("Failed to fetch address book: {}", e.getMessage());
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ logger.warn("Address book update interrupted", e);
} catch (Exception e) {
logger.warn("Failed to update address book", e);
}🤖 Prompt for AI Agents
In sdk/src/main/java/com/hedera/hashgraph/sdk/Client.java around lines 1407 to
1437, the generic Exception catch will swallow InterruptedException thrown by
setNetworkFromAddressBook and fail to restore the thread's interrupt status;
explicitly catch InterruptedException before the generic catch, call
Thread.currentThread().interrupt() to restore the interrupt status, log or
wrap/rethrow if needed, and then return (or rethrow as an unchecked exception)
so the interrupt is not lost; keep the existing TimeoutException and generic
Exception handlers after this new InterruptedException handler.
...ion/java/com/hedera/hashgraph/sdk/test/integration/NodeUpdateTransactionIntegrationTest.java
Outdated
Show resolved
Hide resolved
emiliyank
force-pushed
the
feature/hip-1299-changing-node-account-id
branch
from
45410b7 to
ddadcde
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
sdk/src/main/java/com/hedera/hashgraph/sdk/Client.java (1)
1430-1434: InterruptedException handling still not addressed.The
setNetworkFromAddressBookmethod at line 1426 declaresthrows InterruptedException, but when caught by the genericExceptioncatch block at line 1432, the thread's interrupt status is not restored. This was flagged in a previous review and remains unaddressed.Apply this diff to properly handle
InterruptedException:} catch (TimeoutException e) { logger.warn("Failed to fetch address book: {}", e.getMessage()); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + logger.warn("Address book update interrupted", e); } catch (Exception e) { logger.warn("Failed to update address book", e); }sdk/src/testIntegration/java/com/hedera/hashgraph/sdk/test/integration/NodeUpdateTransactionIntegrationTest.java (1)
193-196: Remove debugSystem.out.printlnstatements.These debug print statements should be removed from the test code. This was flagged in a previous review and remains unaddressed.
- System.out.println("Transaction node: " + resp.nodeId); - System.out.println("Receipt query nodes: " + resp.getReceiptQuery().getNodeAccountIds()); - System.out.println("Client network: " + client.getNetwork()); receipt = resp.setValidateStatus(true).getReceipt(client);
🧹 Nitpick comments (2)
sdk/src/testIntegration/java/com/hedera/hashgraph/sdk/test/integration/NodeUpdateTransactionIntegrationTest.java (1)
4-6: Remove redundant imports.Line 4 uses a wildcard import
import static org.assertj.core.api.Assertions.*;which already includesassertThatandassertThatThrownBy. Lines 5-6 are redundant.-import static org.assertj.core.api.Assertions.*; -import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatThrownBy;Alternatively, keep only the wildcard import if you prefer brevity.
sdk/src/main/java/com/hedera/hashgraph/sdk/Executable.java (1)
789-812: Async path also uses synchronous address book update.Similar to the sync path, the
updateNetworkFromAddressBook()call at line 802 is synchronous. In an async context, this blocks the CompletableFuture chain. Consider whether this should use an async variant (e.g.,CompletableFuture.runAsync(() -> client.updateNetworkFromAddressBook(), client.executor)).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
sdk/src/main/java/com/hedera/hashgraph/sdk/Client.java(1 hunks)sdk/src/main/java/com/hedera/hashgraph/sdk/Executable.java(12 hunks)sdk/src/testIntegration/java/com/hedera/hashgraph/sdk/test/integration/NodeUpdateTransactionIntegrationTest.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
sdk/src/testIntegration/java/com/hedera/hashgraph/sdk/test/integration/NodeUpdateTransactionIntegrationTest.java (3)
sdk/src/main/java/com/hedera/hashgraph/sdk/Client.java (1)
Client(39-1677)sdk/src/main/java/com/hedera/hashgraph/sdk/PrivateKey.java (1)
PrivateKey(20-460)sdk/src/main/java/com/hedera/hashgraph/sdk/Hbar.java (1)
Hbar(16-253)
sdk/src/main/java/com/hedera/hashgraph/sdk/Client.java (1)
sdk/src/main/java/com/hedera/hashgraph/sdk/FileId.java (1)
FileId(14-304)
🔇 Additional comments (10)
sdk/src/testIntegration/java/com/hedera/hashgraph/sdk/test/integration/NodeUpdateTransactionIntegrationTest.java (2)
568-602: LGTM!Good extraction of helper methods to reduce code duplication across test methods. The
createAccount,updateNodeAccountId, andexecuteAccountCreatehelpers improve test readability and maintainability.
70-125: LGTM!Well-structured test with clear Given/When/Then documentation. The test properly verifies that updating a node's account ID with correct signatures succeeds.
sdk/src/main/java/com/hedera/hashgraph/sdk/Executable.java (8)
405-408: LGTM!Correctly advances to the next node before retrying after a connection failure. This ensures the retry uses a different node.
427-434: LGTM!Proper advancement to next node before retrying when an exception requires retry.
460-465: LGTM!Correctly advances request after applying delay in the RETRY case.
695-703: LGTM!Good documentation clarifying that
advanceRequest()is now called explicitly in the retry logic after error detection, rather than automatically ingetRequestForExecute().
743-753: LGTM!Async path correctly advances to the next node before retrying after connection failure, consistent with the sync path.
905-914: LGTM!Correct mapping of
INVALID_NODE_ACCOUNTtoSERVER_ERRORexecution state. This ensures the SDK immediately retries with the next node when a node's account ID has changed, as per HIP-1299.
1013-1018: LGTM!Correct handling: for
INVALID_NODE_ACCOUNT, the node's backoff is not decreased because it will be marked as unhealthy. For other statuses, backoff is decreased as before.
1045-1056: LGTM!Correctly suppresses the SERVER_ERROR warning for
INVALID_NODE_ACCOUNTsince it's handled with TRACE-level logging in the caller afteradvanceRequest().
Signed-off-by: emiliyank <[email protected]>
emiliyank
force-pushed
the
feature/hip-1299-changing-node-account-id
branch
from
ddadcde to
cda1929
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdk/src/main/java/com/hedera/hashgraph/sdk/Executable.java (1)
790-812: Synchronous call in async path may block executor thread.The
updateNetworkFromAddressBook()call at line 802 is synchronous and will block the executor thread while fetching the address book. In the async execution path, this could impact throughput if multiple requests encounterINVALID_NODE_ACCOUNTsimultaneously.Also, the comment at line 801 incorrectly states "async".
Consider making this truly async in the async path:
- // Schedule async address book update - client.updateNetworkFromAddressBook(); + // Update address book asynchronously to avoid blocking + CompletableFuture.runAsync( + client::updateNetworkFromAddressBook, + client.executor + );Alternatively, if synchronous behavior is intentional, update the comment to reflect this.
♻️ Duplicate comments (3)
sdk/src/main/java/com/hedera/hashgraph/sdk/Client.java (1)
1407-1437: InterruptedException handling still loses thread interrupt status.The
setNetworkFromAddressBookmethod declaresthrows InterruptedException(line 539), but when caught by the genericExceptioncatch block at line 1432, the thread's interrupt status is not restored. This was flagged in a previous review but hasn't been addressed.Apply this diff to properly handle
InterruptedException:} catch (TimeoutException e) { logger.warn("Failed to fetch address book: {}", e.getMessage()); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + logger.warn("Address book update interrupted", e); } catch (Exception e) { logger.warn("Failed to update address book", e); }sdk/src/main/java/com/hedera/hashgraph/sdk/Executable.java (1)
440-456: Address book update is synchronous, not async.The comment at line 451 says "Schedule async address book update" but
updateNetworkFromAddressBook()is synchronous (blocks on network round-trip). This was flagged in a previous review.Update the comment to reflect the synchronous nature:
- // Schedule async address book update + // Synchronously update address book (blocks on network call) client.updateNetworkFromAddressBook();sdk/src/testIntegration/java/com/hedera/hashgraph/sdk/test/integration/NodeUpdateTransactionIntegrationTest.java (1)
180-182: Remove debugSystem.out.printlnstatements.These debug print statements should be removed from test code. Use the test framework's logging or assertions if this information is needed for debugging.
- System.out.println("Transaction node: " + resp.nodeId); - System.out.println("Receipt query nodes: " + resp.getReceiptQuery().getNodeAccountIds()); - System.out.println("Client network: " + client.getNetwork()); var receipt = resp.setValidateStatus(true).getReceipt(client);
🧹 Nitpick comments (2)
sdk/src/testIntegration/java/com/hedera/hashgraph/sdk/test/integration/NodeUpdateTransactionIntegrationTest.java (2)
4-6: Redundant static imports.Line 4 uses a wildcard import
import static org.assertj.core.api.Assertions.*;which already includesassertThatandassertThatThrownBy. Lines 5 and 6 are redundant.-import static org.assertj.core.api.Assertions.*; -import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatThrownBy;Alternatively, keep only the wildcard import if you prefer brevity.
186-188: Consider replacing fixed sleep with polling mechanism.
Thread.sleep(10000)is a brittle approach for waiting on mirror node data import. The timing may vary across environments, leading to flaky tests.Consider implementing a polling mechanism with timeout, or extracting this into a reusable helper method that polls for the expected state. This pattern appears in multiple tests (lines 187, 437, 480).
private void waitForMirrorNodeSync(Client client, Duration timeout) throws Exception { var deadline = Instant.now().plus(timeout); while (Instant.now().isBefore(deadline)) { try { // Attempt to verify mirror node has synced client.updateNetworkFromAddressBook(); return; } catch (Exception e) { Thread.sleep(1000); } } throw new TimeoutException("Mirror node did not sync within timeout"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
sdk/src/main/java/com/hedera/hashgraph/sdk/Client.java(1 hunks)sdk/src/main/java/com/hedera/hashgraph/sdk/Executable.java(12 hunks)sdk/src/testIntegration/java/com/hedera/hashgraph/sdk/test/integration/NodeUpdateTransactionIntegrationTest.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
sdk/src/testIntegration/java/com/hedera/hashgraph/sdk/test/integration/NodeUpdateTransactionIntegrationTest.java (3)
sdk/src/main/java/com/hedera/hashgraph/sdk/Client.java (1)
Client(39-1677)sdk/src/main/java/com/hedera/hashgraph/sdk/PrivateKey.java (1)
PrivateKey(20-460)sdk/src/main/java/com/hedera/hashgraph/sdk/Hbar.java (1)
Hbar(16-253)
🔇 Additional comments (6)
sdk/src/testIntegration/java/com/hedera/hashgraph/sdk/test/integration/NodeUpdateTransactionIntegrationTest.java (2)
501-535: Good test helper extraction.The
createAccount,updateNodeAccountId, andexecuteAccountCreatehelper methods effectively reduce code duplication and improve test readability.
68-118: Test appropriately disabled with clear reasoning.The test
shouldSucceedWhenUpdatingNodeAccountIdWithProperSignaturesis marked@Disabled, which is appropriate while the underlying functionality is being implemented or debugged. The test structure and assertions look correct.sdk/src/main/java/com/hedera/hashgraph/sdk/Executable.java (4)
905-908: Correct mapping of INVALID_NODE_ACCOUNT to SERVER_ERROR.Mapping
INVALID_NODE_ACCOUNTtoSERVER_ERRORtriggers immediate retry with the next node, which is the correct behavior when a node's account ID has changed. The comment clearly explains the rationale.
1013-1056: Well-structured handling of INVALID_NODE_ACCOUNT in response processing.The changes correctly:
- Skip
decreaseBackoffforINVALID_NODE_ACCOUNTsince the node will be marked unhealthy elsewhere- Suppress redundant warning logs for
INVALID_NODE_ACCOUNTin theSERVER_ERRORcase- Add clear comments explaining the behavior alignment with Go SDK
695-703: Good refactoring of node advancement logic.Moving
advanceRequest()calls to explicit retry points after error detection (rather than ingetRequestForExecute) is a cleaner approach that makes the control flow more predictable.
406-407: Consistent node advancement on connection failure.Adding
advanceRequest()after connection failure detection ensures the next retry uses a different node. This pattern is applied consistently across all retry paths.
Signed-off-by: emiliyank <[email protected]>
Signed-off-by: emiliyank <[email protected]>
Signed-off-by: emiliyank <[email protected]>
Signed-off-by: emiliyank <[email protected]>
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.