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

chore(sdk): Remove node id config option #2777

Merged
merged 4 commits into from
Oct 2, 2024

Conversation

teogeb
Copy link
Contributor

@teogeb teogeb commented Oct 1, 2024

Removed obsolete NetworkNodeConfig#id config option. The NetworkNodeConfig interface corresponds the ContentDeliveryManagerOptions interface in trackerless-network, and there is no id field in that interface. Therefore the config option doesn't configure anything and is obsolete.

Background

The description of the removed config option was "the Ethereum address of the node". Therefore it is likely that the config option is from pre-trackerless-network era. In that implementation node ids were Ethereum addresses.

Other changes

Fixed config injection problem in OperatorPlugin test. When the "invalid configuration" test case modified the config object returned by formConfig, it modified also the TEST_CONFIG object (because TEST_CONFIG object properties are injected by reference in formConfig). That caused another test in the OperatorPlugin to fail.

  • Alternatively we could have done the the cloning in formConfig. But as cloning is needed only in this one test case, maybe better to do it locally.

@teogeb teogeb force-pushed the rm-sdk-config-NetworkNodeConfig-id branch from 39b8b4e to a33acb6 Compare October 1, 2024 21:33
@teogeb teogeb requested review from juslesan and harbu October 1, 2024 21:57
@teogeb teogeb merged commit be54bdd into main Oct 2, 2024
24 checks passed
@teogeb teogeb deleted the rm-sdk-config-NetworkNodeConfig-id branch October 2, 2024 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants