Conversation
This comment was marked as outdated.
This comment was marked as outdated.
|
/run-security-scan |
alexcos20
left a comment
There was a problem hiding this comment.
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.
|
/run-security-scan |
alexcos20
left a comment
There was a problem hiding this comment.
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.
|
/run-security-scan |
|
DOCKERHUB_PASSWORDNONO replacement is to remove Docker login for the moment, as limits are imposed per user |
alexcos20
left a comment
There was a problem hiding this comment.
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.
giurgiur99
left a comment
There was a problem hiding this comment.
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 |
Summary
This PR finishes the
getConfiguration/global state cleanup by moving runtime dependencies toOceanNodeinstance 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
tearDownAll()and safer indexer replacement/cleanup logic.Removed global utility indirection
src/utils/auth.ts,getDatabaseinsrc/utils/database.ts, chain-check/provider wrappers insrc/utils/blockchain.ts, andhasP2PInterfacefromsrc/utils/config.ts).validateDdoHandlerhelper and migrated callers to node-scoped paths.Indexer refactor and robustness
OceanIndexernow receives full config + DB directly and passes them intoChainIndexer.OceanIndexer.stop()to cleanly stop chain indexers and remove listeners.OrderStartedEventProcessorOrderReusedEventProcessorto prevent counting the same event twice when a block range is reprocessed.
Provider fees refactor
Providerfeesclass bound to anOceanNode.Handler/route dependency migration
req.oceanNode.../node...) instead of global utility access.CI and Test Infrastructure
CI workflow (
.github/workflows/ci.yml)ocean.js,ocean-cli, andocean-nodeearlier in the job.DOCKERHUB_*NONO), effectively disabling that login step(due to current dockerhub limits)Test suite updates
tearDownAll) and node-scoped DB/indexer helpers.waitToIndextest helper to receiveOceanNodeexplicitly.Why