Skip to content

refactor getConfiguration#1344

Merged
alexcos20 merged 41 commits intomainfrom
feature/optimize_getConfiguration
Apr 24, 2026
Merged

refactor getConfiguration#1344
alexcos20 merged 41 commits intomainfrom
feature/optimize_getConfiguration

Conversation

@alexcos20
Copy link
Copy Markdown
Member

@alexcos20 alexcos20 commented Apr 22, 2026

Summary

This PR finishes the getConfiguration/global state cleanup by moving runtime dependencies to OceanNode instance APIs, then propagates that model through indexer, handlers, compute flows, and tests.
It also includes indexer stability fixes (cache reset + duplicate event protection) and CI/system-test workflow updates.

Major Changes

  • OceanNode lifecycle and runtime ownership

    • Added node-scoped helpers for admin resolution, chain/provider checks, DB access, P2P capability, and validation signatures.
    • Added tearDownAll() and safer indexer replacement/cleanup logic.
    • Improved singleton behavior to support explicit refresh in tests and avoid stale shared state.
  • Removed global utility indirection

    • Removed utility APIs that fetched config/DB globally (src/utils/auth.ts, getDatabase in src/utils/database.ts, chain-check/provider wrappers in src/utils/blockchain.ts, and hasP2PInterface from src/utils/config.ts).
    • Deleted legacy validateDdoHandler helper and migrated callers to node-scoped paths.
  • Indexer refactor and robustness

    • OceanIndexer now receives full config + DB directly and passes them into ChainIndexer.
    • Event processors are instantiated with explicit config and processor cache can be cleared on indexer startup.
    • Added OceanIndexer.stop() to cleanly stop chain indexers and remove listeners.
    • Added duplicate tx guards in:
      • OrderStartedEventProcessor
      • OrderReusedEventProcessor
        to prevent counting the same event twice when a block range is reprocessed.
  • Provider fees refactor

    • Converted fee helper functions into Providerfees class bound to an OceanNode.
    • Updated compute/handler call sites and tests to use class instance methods.
    • Kept existing provider fee signing/verification behavior while removing dependency on global config loaders.
  • Handler/route dependency migration

    • Updated core/admin/HTTP/P2P handlers to consistently use node-owned config/DB (req.oceanNode... / node...) instead of global utility access.
    • Standardized DB access in nonce/auth/query/ddo/download-related paths.

CI and Test Infrastructure

  • CI workflow (.github/workflows/ci.yml)

    • Reordered system-test setup to build/link ocean.js, ocean-cli, and ocean-node earlier in the job.
    • Reduced Ocean Node readiness polling window for system tests.
    • Note: Docker Hub login condition was switched to placeholder env names (DOCKERHUB_*NONO), effectively disabling that login step(due to current dockerhub limits)
  • Test suite updates

    • Refactored many integration tests to explicit node lifecycle management (tearDownAll) and node-scoped DB/indexer helpers.
    • Updated waitToIndex test helper to receive OceanNode explicitly.
    • Adjusted auth/fee/indexer-related tests to align with the new APIs.
    • Formatting and organization improvements across test sections.

Why

  • Reduce hidden global state and config/database drift across subsystems.
  • Make runtime ownership explicit and deterministic (especially in tests and indexer startup paths).
  • Improve indexer correctness under retries/reprocessing by avoiding duplicate order aggregation.
  • Keep CI/system tests faster and more reproducible.

@alexcos20

This comment was marked as outdated.

alexcos20

This comment was marked as outdated.

@alexcos20 alexcos20 linked an issue Apr 22, 2026 that may be closed by this pull request
@alexcos20
Copy link
Copy Markdown
Member Author

/run-security-scan

Copy link
Copy Markdown
Member Author

@alexcos20 alexcos20 left a comment

Choose a reason for hiding this comment

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

AI automated code review (Gemini 3).

Overall risk: medium

Summary:
This PR performs an excellent and massive architectural refactor to remove global state (such as getConfiguration and getDatabase), moving configuration and DB instances directly into the OceanNode instance and relevant classes. This drastically improves testability and performance. However, there are some debugging artifacts left in the CI workflow that will break the build, and a couple of missing null checks for the P2P node when the P2P interface is disabled.

Comments:
• [ERROR][bug] It looks like some debugging artifacts were accidentally committed. The NONO suffix will break the Docker Hub login step, and lowering the health check loop from 90 to 12 iterations might cause flaky CI failures if the node takes slightly longer to boot.

       - name: Login to Docker Hub
-        if: ${{ env.DOCKERHUB_PASSWORDNONO && env.DOCKERHUB_USERNAMENONO }}
+        if: ${{ env.DOCKERHUB_PASSWORD && env.DOCKERHUB_USERNAME }}

And for the loop at line 321:

       - name: Check Ocean Node is running
         run: |
-          for i in $(seq 1 12); do
+          for i in $(seq 1 90); do

• [WARNING][bug] If the Ocean Node is configured without a P2P interface, node.getP2PNode() will return null/undefined, which will cause p2pNode.advertiseString(...) to throw a TypeError further down. We should safeguard against this.

   try {
     const db = await node.getDatabase()
     const p2pNode = node.getP2PNode()
-    if (!db || !db.ddo) {
+    if (!db || !db.ddo || !p2pNode) {
       P2P_LOGGER.info(
-        'Database or DDO database not available. Cannot announce DDOS.'
+        'Database, DDO database, or P2P node not available. Cannot announce DDOS.'
       )
       return
     }

• [WARNING][bug] If the node is running with P2P disabled (hasP2P is false), getP2PNode() can be null, causing getNetworkingStats() to throw an error. It's safer to handle this gracefully.

       const node = this.getOceanNode()
       const config = node.getConfig()
       if (config.p2pConfig.enableNetworkStats) {
-        const stats = node.getP2PNode().getNetworkingStats()
+        const p2pNode = node.getP2PNode()
+        if (!p2pNode) {
+          return {
+            stream: null,
+            status: { httpStatus: 503, error: 'P2P Interface is disabled' }
+          }
+        }
+        const stats = p2pNode.getNetworkingStats()
         return {

• [INFO][style] Since Purgatory.getInstance was refactored to be synchronous and return the instance directly, the await keyword is no longer necessary here. You can safely remove it here as well as in src/test/integration/purgatory.test.ts.

-      const purgatory = await Purgatory.getInstance(this.getConfig())
+      const purgatory = Purgatory.getInstance(this.getConfig())

• [INFO][style] These commented-out lines for DB re-initialization should be removed to keep the codebase clean and maintainable.

         INDEXER_LOGGER.info(
           'Database connection stable - reinitialising DB and restarting all chain indexers'
         )
-        // const freshDb = await getDatabase(true)
-        // if (freshDb) {
-        // this.db = freshDb
-        // }
 
         await this.startAllChainIndexers()

• [INFO][other] Great addition here and in OrderStartedEventProcessor. Checking existingOrder establishes idempotency and avoids duplicating state when indexing identical events. Excellent implementation quality.

@alexcos20
Copy link
Copy Markdown
Member Author

/run-security-scan

Copy link
Copy Markdown
Member Author

@alexcos20 alexcos20 left a comment

Choose a reason for hiding this comment

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

AI automated code review (Gemini 3).

Overall risk: medium

Summary:
This PR optimizes configuration retrieval by centralizing it through OceanNode.getInstance().getConfig() and passing it down via dependency injection. It eliminates numerous redundant async File I/O operations from getConfiguration(). The PR also introduces robust idempotency checks in the Indexer processors and improves the address validation logic. However, there are a few issues: a leftover debugging modification in the GitHub Actions CI workflow, a missing safety check for unsupported chain IDs in the new Providerfees class, and a potential TypeScript context error in handleProtocolCommands.

Comments:
• [ERROR][bug] It looks like this was an unintentional change made during local testing or debugging. This disables Docker Hub login, which will break subsequent Docker image build/push steps in the CI pipeline. Please revert it.

-        if: ${{ env.DOCKERHUB_PASSWORDNONO && env.DOCKERHUB_USERNAMENONO }}
+        if: ${{ env.DOCKERHUB_PASSWORD && env.DOCKERHUB_USERNAME }}

• [ERROR][bug] Referencing this.config in an exported standalone function will cause TypeScript to complain about this being implicitly any (if noImplicitThis is enabled) and could lead to runtime errors if the context gets lost. It is safer to retrieve the config via the singleton.

-  const configuration = this.config
+  const configuration = OceanNode.getInstance().getConfig()

• [WARNING][bug] If an asset specifies a chainId that is not configured or supported by this node, this.node.getBlockchain(assetChainId) will return undefined. Calling .getProvider() on undefined will crash the application with a TypeError. You should verify that the blockchain exists before attempting to access its provider.

     if (providerFeeToken && providerFeeToken?.toLowerCase() !== ZeroAddress) {
-      const provider = await this.node.getBlockchain(assetChainId).getProvider()
+      const blockchain = this.node.getBlockchain(Number(assetChainId))
+      if (!blockchain) {
+        throw new Error(`Blockchain for chainId ${assetChainId} is not configured`)
+      }
+      const provider = await blockchain.getProvider()
       const decimals = await getDatatokenDecimals(providerFeeToken, provider)

• [WARNING][performance] Lazy initialization of the database without a Promise lock can cause a race condition. If getDatabase() is called concurrently at node startup by different components, it will invoke Database.init(dbConfig) multiple times, potentially leading to SQLite lock errors (SQLITE_BUSY) or memory leaks. Consider storing the initialization Promise.

+  private dbInitPromise: Promise<Database> | null = null
+
   async getDatabase(forceReload: boolean = false): Promise<Database> {
     if (!this.database || forceReload) {
-      const { dbConfig } = this.config
-      if (dbConfig && dbConfig.url) {
-        this.database = await Database.init(dbConfig)
-      }
+      if (!this.dbInitPromise || forceReload) {
+        const { dbConfig } = this.config
+        if (dbConfig && dbConfig.url) {
+          this.dbInitPromise = Database.init(dbConfig).then(db => {
+            this.database = db
+            return db
+          })
+        }
+      }
+      return await this.dbInitPromise
     }
     return this.database
   }

• [INFO][other] tearDownAll is asynchronous, but it's called inside the synchronous getInstance factory without waiting. This can cause the old instance to still be closing database handles and releasing ports (EADDRINUSE) in the background while the new instance immediately tries to claim them. Since this is primarily used for tests, it might not be a severe issue, but consider exposing an async recreateInstance() method to handle this safely in the future.
• [INFO][style] Good use of getAddress() for normalization. Comparing checksummed EVM addresses instead of doing a .toLowerCase().includes() is much more secure, robust, and less prone to substring collision bugs. Excellent change here!
• [INFO][performance] Adding idempotency checks (if (existingOrder) ...) is a great improvement. It protects the database from re-ingesting events during overlaps and saves compute resources.

@alexcos20
Copy link
Copy Markdown
Member Author

/run-security-scan

@alexcos20 alexcos20 self-assigned this Apr 23, 2026
@alexcos20
Copy link
Copy Markdown
Member Author

DOCKERHUB_PASSWORDNONO replacement is to remove Docker login for the moment, as limits are imposed per user

Copy link
Copy Markdown
Member Author

@alexcos20 alexcos20 left a comment

Choose a reason for hiding this comment

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

AI automated code review (Gemini 3).

Overall risk: high

Summary:
This PR introduces valuable architectural changes, reducing reliance on the globally accessed getConfiguration() and tightly coupling components via OceanNode.getInstance().getConfig(). It brings a major security fix regarding authorizedPublishers validation. However, it contains critical issues: CI debugging artifacts left in ci.yml, a severe context issue (this.getConfig()) in the P2P protocol handler, and a missing null check on C2D engines during database cron sweeps.

Comments:
• [ERROR][bug] It looks like debug modifications were accidentally left in the CI workflow. This will prevent Docker Hub authentication from executing successfully. Please revert the NONO suffix.

-        if: ${{ env.DOCKERHUB_PASSWORDNONO && env.DOCKERHUB_USERNAMENONO }}
+        if: ${{ env.DOCKERHUB_PASSWORD && env.DOCKERHUB_USERNAME }}

• [WARNING][other] The startup wait timeout has been significantly reduced (from ~90s to 12s). This may cause the CI pipeline to fail randomly if the Ocean Node takes slightly longer to start. Please revert back to 90 or ensure 12s is universally sufficient for all runners.

-          for i in $(seq 1 12); do
+          for i in $(seq 1 90); do

• [ERROR][bug] handleProtocolCommands is a standalone function, not a class method. Using this.getConfig() will result in a runtime error (TypeError: this.getConfig is not a function) or undefined context when called via libp2p. You should access the config via the singleton instance.

-  const configuration = this.getConfig()
+  const configuration = OceanNode.getInstance().getConfig()

• [ERROR][bug] getC2DEngines() can return undefined if compute engines are disabled. Accessing .engines directly on it will cause a TypeError and crash the cronjob process. Add safe navigation or a conditional check.

-    const allEngines = await OceanNode.getInstance().getC2DEngines().engines
+    const c2dEngines = OceanNode.getInstance().getC2DEngines()
+    const allEngines = c2dEngines ? c2dEngines.engines : []

• [INFO][style] Standard naming conventions suggest using PascalCase for classes. Consider renaming Providerfees to ProviderFees for consistency.

-export class Providerfees {
+export class ProviderFees {

• [WARNING][architecture] Since getInstance is synchronous, running tearDownAll asynchronously using .catch() might lead to port conflicts or SQLite database locks (EBUSY) while the old instance shuts down its indexer/engines and the new instance starts up simultaneously. If modifying getInstance to be async is out of scope, consider logging a warning specifically around potential resource locks.
• [INFO][security] Great catch here. Using getAddress for an exact string/checksum comparison instead of .includes fixes a significant security vulnerability where similar-looking or substring addresses could incorrectly bypass the authorization check.

@alexcos20 alexcos20 marked this pull request as ready for review April 23, 2026 07:38
Copy link
Copy Markdown
Member

@ndrpp ndrpp left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Copy Markdown
Contributor

@giurgiur99 giurgiur99 left a comment

Choose a reason for hiding this comment

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

Just some small notes

There are still some places in which we await for getInstance() but now it's sync

Comment thread .github/workflows/ci.yml
@alexcos20
Copy link
Copy Markdown
Member Author

Just some small notes

There are still some places in which we await for getInstance() but now it's sync

I think we might need some async calls in getInstance in the future, so I will leave it for now

@alexcos20 alexcos20 merged commit a18083e into main Apr 24, 2026
11 checks passed
@alexcos20 alexcos20 deleted the feature/optimize_getConfiguration branch April 24, 2026 14:11
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.

Unexpected configuration changes

3 participants