Skip to content

Conversation

@ivaylonikolov7
Copy link
Contributor

@ivaylonikolov7 ivaylonikolov7 commented Nov 5, 2025

Summary

This PR implements the test plan for validating updates to the
account_id field in NodeUpdateTransactionBody as part of the Dynamic
Address Book (DAB) enhancement. It also covers new SDK behaviors that
improve reliability when node account IDs become outdated or invalid.

Scope

Test coverage for account_id updates

  • Verifies successful updates to a node's account_id.
  • Tests rejection of invalid or improperly formatted account IDs.
  • Ensures only authorized entities can perform the update.
  • Confirms no regressions when the field is unchanged.

SDK node-selection and retry behavior

  1. Signing with all nodes
    Ensures the SDK signs transactions with all nodes to reduce the
    chance of failures caused by outdated node account IDs.

  2. Client network refresh via mirror node
    When an invalid node account error occurs, the SDK retrieves updated
    network information from a mirror node and retries the transaction.

  3. Fallback behavior without a mirror network
    If no mirror node is configured, the SDK retries execution using the
    next node ID in the transaction's node list until all options are
    exhausted.

Alignment

The implementation aligns with the design described in:
"Changing of Node Account IDs for Nodes" (sdk-collaboration-hub#53)

Rationale

These tests ensure the robustness of Dynamic Address Book functionality,
reliable handling of account ID changes, and improved resilience in
transaction execution workflows.

Related issue(s):
#3370

Fixes #3370

@lfdt-bot
Copy link
Contributor

lfdt-bot commented Nov 5, 2025

Snyk checks have passed. No issues have been found so far.

Status Scanner Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@venilinvasilev venilinvasilev force-pushed the feat/changing-node-account-ids branch from 55e9bfb to 62cb6b7 Compare November 10, 2025 09:52
@venilinvasilev venilinvasilev self-assigned this Nov 10, 2025
@codecov
Copy link

codecov bot commented Nov 10, 2025

Codecov Report

❌ Patch coverage is 20.51282% with 31 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/Executable.js 8.82% 31 Missing ⚠️
Files with missing lines Coverage Δ
src/channel/WebChannel.js 86.69% <100.00%> (+0.10%) ⬆️
src/client/Network.js 84.55% <100.00%> (-0.30%) ⬇️
src/transaction/Transaction.js 92.29% <100.00%> (+<0.01%) ⬆️
src/Executable.js 83.56% <8.82%> (-3.13%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@venilinvasilev venilinvasilev force-pushed the feat/changing-node-account-ids branch 6 times, most recently from be6f7db to 2cf661a Compare November 13, 2025 15:34
@venilinvasilev venilinvasilev changed the title wip: changing node account ids feat: changing node account ids Nov 14, 2025
@venilinvasilev venilinvasilev marked this pull request as ready for review November 14, 2025 08:43
@venilinvasilev venilinvasilev requested review from a team as code owners November 14, 2025 08:43
Signed-off-by: venilinvasilev <venilin.vasilev@gmail.com>
Signed-off-by: venilinvasilev <venilin.vasilev@gmail.com>
Signed-off-by: venilinvasilev <venilin.vasilev@gmail.com>
Signed-off-by: venilinvasilev <venilin.vasilev@gmail.com>
Signed-off-by: venilinvasilev <venilin.vasilev@gmail.com>
Signed-off-by: venilinvasilev <venilin.vasilev@gmail.com>
Signed-off-by: venilinvasilev <venilin.vasilev@gmail.com>
Signed-off-by: venilinvasilev <venilin.vasilev@gmail.com>
Signed-off-by: venilinvasilev <venilin.vasilev@gmail.com>
@venilinvasilev venilinvasilev force-pushed the feat/changing-node-account-ids branch from 1e6bf46 to 5e5fa1b Compare November 17, 2025 15:10
@ivaylonikolov7
Copy link
Contributor Author

ivaylonikolov7 commented Nov 17, 2025

LGTM. Great job that you also added browser tests for that. 🙌 Won't approve it officially until we have a stable tag for that.

One small thing that I would change is no need to set the client operator explicitly to 0.0.2 in the test suite itself when you can add it in the build.yml file. It's like we are doing extra steps there.

Signed-off-by: ivaylonikolov7 <ivaylo.nikolov@limechain.tech>
venilinvasilev
venilinvasilev previously approved these changes Nov 27, 2025
Copy link
Contributor

@venilinvasilev venilinvasilev left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Ivaylo Nikolov <ivaylo.nikolov@limechain.tech>
@ivaylonikolov7 ivaylonikolov7 merged commit 4e3e847 into main Nov 28, 2025
17 of 20 checks passed
@ivaylonikolov7 ivaylonikolov7 deleted the feat/changing-node-account-ids branch November 28, 2025 10:24
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.

Changing Node Account Ids

6 participants