Next major release - V9.0 - #2110
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR updates workflow runtimes and release metadata, adds service-on-demand types and provider methods with tests, and introduces escrow batch and relocking support with contract tests. ChangesRelease and Workflow Updates
Service-on-Demand API
Escrow Batch Operations
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
47-52: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winSet
persist-credentials: falseon Barge checkout steps.Both Barge checkout steps use
actions/checkout@v3withoutpersist-credentials: false, leaving theGITHUB_TOKENin the local.git/config. While current artifacts are uploaded fromcoverage/(not the barge directory), disabling credential persistence is a low-cost security hardening since no git push-back to the barge repo is needed.🔒 Proposed fix for both Barge checkout steps
- name: Checkout Barge uses: actions/checkout@v3 with: repository: "oceanprotocol/barge" path: "barge" ref: "feature/node-v4" + persist-credentials: falseAlso applies to: 132-137
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 47 - 52, Add persist-credentials: false to both Barge checkout steps that use actions/checkout@v3 in the workflow, including the Checkout Barge step and the second matching Barge checkout block, so the GITHUB_TOKEN is not written into the local .git/config. Keep the existing repository, path, and ref settings unchanged; just update the checkout configuration for those steps.Source: Linters/SAST tools
🧹 Nitpick comments (4)
.github/workflows/ci.yml (1)
52-52: 🩺 Stability & Availability | 🔵 TrivialPin Barge to a stable ref before merging to
main.Both
test_unitandtest_integrationjobs pin Barge toref: "feature/node-v4", a mutable feature branch. If that branch is force-pushed or deleted, CI will break unpredictably. This is acceptable for theDoNotMergerelease-prep phase, but should be switched to a tag or stable branch before the v9.0 release merge.Also applies to: 137-137
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml at line 52, The CI workflow still references a mutable Barge branch via the pinned ref used by the test jobs, so update the Barge checkout in the workflow to a stable, immutable ref (such as a release tag or permanent branch) before merging. Make the change in the workflow sections for the affected jobs, using the existing Barge ref setting so it’s easy to locate and keep both test jobs consistent.src/@types/Services.ts (2)
76-83: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLoosely-typed catch-all weakens the declared fields.
[key: string]: anyonServiceJobPaymentdefeats the point of declaringchainId,token,lockTx, etc. as typed fields — any consumer code can silently pass/receive mistyped values through this fallback. Preferunknownfor the index signature so callers must narrow before use, preserving forward-compatibility without giving up type safety.♻️ Proposed fix
export interface ServiceJobPayment { chainId?: number token?: string lockTx?: string claimTx?: string cost?: string | number - [key: string]: any + [key: string]: unknown }As per coding guidelines, "Always specify types and do not allow implicit
any."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/`@types/Services.ts around lines 76 - 83, The `ServiceJobPayment` interface is weakened by the `[key: string]: any` catch-all, which bypasses the explicit typed fields like `chainId`, `token`, `lockTx`, `claimTx`, and `cost`. Update the index signature in `ServiceJobPayment` to use `unknown` instead of `any`, so callers must narrow extra properties before use while keeping the declared fields strongly typed and preserving forward compatibility.Source: Coding guidelines
86-111: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
resources: any[]discards a known type.The comment states the real node-side type is
ComputeResourceRequestWithPrice[], but the client field is typedany[], losing all type safety for consumers inspecting job resources.As per coding guidelines (
src/@types/**/*.ts), "Define clear interfaces for all public APIs."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/`@types/Services.ts around lines 86 - 111, The ServiceJob interface currently uses resources: any[], which drops the known node-side type and weakens the public API. Update the resources field in ServiceJob to use the concrete ComputeResourceRequestWithPrice[] type (or import and reference the shared type directly) so consumers of ServiceJob get proper type safety; locate this in the ServiceJob definition alongside other typed fields like endpoints and payment.Source: Coding guidelines
src/services/providers/P2pProvider.ts (1)
1767-1902: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing JSDoc on new public service methods.
getServiceTemplates,serviceStart,serviceStop,serviceExtend,serviceRestart, andgetServiceStatushave no JSDoc, unlike almost every other public method in this file and their HttpProvider counterparts (which all document params/returns).As per coding guidelines, "Add JSDoc comments for all public APIs and document optional versus required parameters."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/providers/P2pProvider.ts` around lines 1767 - 1902, The new public APIs in P2pProvider are missing JSDoc, unlike the rest of the class and the HttpProvider equivalents. Add JSDoc blocks for getServiceTemplates, serviceStart, serviceStop, serviceExtend, serviceRestart, and getServiceStatus, documenting each parameter (especially optional ones like chainId, signal, serviceId, userData) and the return type so the public contract matches the file’s existing style.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 47-52: Add persist-credentials: false to both Barge checkout steps
that use actions/checkout@v3 in the workflow, including the Checkout Barge step
and the second matching Barge checkout block, so the GITHUB_TOKEN is not written
into the local .git/config. Keep the existing repository, path, and ref settings
unchanged; just update the checkout configuration for those steps.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Line 52: The CI workflow still references a mutable Barge branch via the
pinned ref used by the test jobs, so update the Barge checkout in the workflow
to a stable, immutable ref (such as a release tag or permanent branch) before
merging. Make the change in the workflow sections for the affected jobs, using
the existing Barge ref setting so it’s easy to locate and keep both test jobs
consistent.
In `@src/`@types/Services.ts:
- Around line 76-83: The `ServiceJobPayment` interface is weakened by the `[key:
string]: any` catch-all, which bypasses the explicit typed fields like
`chainId`, `token`, `lockTx`, `claimTx`, and `cost`. Update the index signature
in `ServiceJobPayment` to use `unknown` instead of `any`, so callers must narrow
extra properties before use while keeping the declared fields strongly typed and
preserving forward compatibility.
- Around line 86-111: The ServiceJob interface currently uses resources: any[],
which drops the known node-side type and weakens the public API. Update the
resources field in ServiceJob to use the concrete
ComputeResourceRequestWithPrice[] type (or import and reference the shared type
directly) so consumers of ServiceJob get proper type safety; locate this in the
ServiceJob definition alongside other typed fields like endpoints and payment.
In `@src/services/providers/P2pProvider.ts`:
- Around line 1767-1902: The new public APIs in P2pProvider are missing JSDoc,
unlike the rest of the class and the HttpProvider equivalents. Add JSDoc blocks
for getServiceTemplates, serviceStart, serviceStop, serviceExtend,
serviceRestart, and getServiceStatus, documenting each parameter (especially
optional ones like chainId, signal, serviceId, userData) and the return type so
the public contract matches the file’s existing style.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: baddc169-a715-4377-a76e-acb5fb5f46a6
📒 Files selected for processing (11)
.github/workflows/ci.ymlsrc/@types/Compute.tssrc/@types/Provider.tssrc/@types/Services.tssrc/@types/index.tssrc/services/providers/BaseProvider.tssrc/services/providers/HttpProvider.tssrc/services/providers/P2pProvider.tstest/integration/ComputeFlow.test.tstest/integration/Services.test.tstest/unit/Services.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/@types/index.ts
* update dep to contracts v2.9.0 * use new functions & types * fix * fix comments * force contracts version in barge
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/contracts/Escrow.ts (1)
365-367: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate
authorizereturn type to includenull.
authorizeTxalready declaresPromise<TransactionRequest | null>and returnsnullwhen the payee is already authorized. The new guard at line 365 propagates this as an explicitnullreturn, but the method signaturePromise<ReceiptOrEstimate<G>>doesn't reflect it. Callers have no type-level signal thatnullis a valid, expected return — only a runtime surprise.♻️ Proposed type fix
- ): Promise<ReceiptOrEstimate<G>> { + ): Promise<ReceiptOrEstimate<G> | null> { const tx = await this.authorizeTx(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/contracts/Escrow.ts` around lines 365 - 367, The `authorize` method in `Escrow` now has an explicit `null` path via the `!tx` guard, but its return type still omits that possibility. Update the `authorize` signature so callers see `null` in the type contract, and make sure the change stays consistent with `authorizeTx` and the `ReceiptOrEstimate<G>` usage where the early return occurs.test/unit/Escrow.test.ts (1)
267-283: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a negative test for
assertSingleTokenForDecimalsOverride.The mismatched-arrays test covers
reLocksTxlength validation, but there's no test verifying thatbundleorreLocksrejects calls wheretokenDecimalsis provided alongside multiple different tokens. This validation guards against incorrect decimal conversion that could send wrong amounts to the contract.🧪 Suggested test
it('should reject bundle with multiple tokens when tokenDecimals is set', async () => { try { await Escrow.bundle( [{ token: OCEAN, amount: '1' }], [], [{ token: addresses.Datatoken || OCEAN, payee: await user1.getAddress(), maxLockedAmount: '1', maxLockSeconds: '100', maxLockCounts: '3' }], 18 ) assert(false, 'expected bundle to throw on multiple tokens with tokenDecimals') } catch (error) { assert( error.message.includes('multiple different tokens'), `unexpected error message: ${error.message}` ) } })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/Escrow.test.ts` around lines 267 - 283, Add a negative test around Escrow.bundle or Escrow.reLocks to cover assertSingleTokenForDecimalsOverride when tokenDecimals is supplied with multiple different tokens. Use the existing Escrow.bundle and/or Escrow.reLocks entry points with mixed token inputs and a decimals override, then assert the call throws an error mentioning multiple different tokens or equivalent validation text. Keep the existing mismatched-arrays test intact and place the new case near it in Escrow.test.ts so the token-decimals guard is explicitly verified.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/contracts/Escrow.ts`:
- Around line 365-367: The `authorize` method in `Escrow` now has an explicit
`null` path via the `!tx` guard, but its return type still omits that
possibility. Update the `authorize` signature so callers see `null` in the type
contract, and make sure the change stays consistent with `authorizeTx` and the
`ReceiptOrEstimate<G>` usage where the early return occurs.
In `@test/unit/Escrow.test.ts`:
- Around line 267-283: Add a negative test around Escrow.bundle or
Escrow.reLocks to cover assertSingleTokenForDecimalsOverride when tokenDecimals
is supplied with multiple different tokens. Use the existing Escrow.bundle
and/or Escrow.reLocks entry points with mixed token inputs and a decimals
override, then assert the call throws an error mentioning multiple different
tokens or equivalent validation text. Keep the existing mismatched-arrays test
intact and place the new case near it in Escrow.test.ts so the token-decimals
guard is explicitly verified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7a3bf21a-e0fe-4cc1-9ea1-af98233f2eae
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
.github/workflows/ci.ymlpackage.jsonsrc/@types/Escrow.tssrc/@types/index.tssrc/contracts/Escrow.tstest/unit/Escrow.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/ci.yml
- src/@types/index.ts
* sign issuer peer id * add pr with new signature for auth tokens * fix validate ddo * fix review comments --------- Co-authored-by: alexcos20 <alex.coseru@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/services/providers/P2pProvider.ts (1)
1774-1909: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd JSDoc to the new public service methods. Unlike the HttpProvider counterparts, these public APIs (
getServiceTemplates,serviceStart,serviceStop,serviceExtend,serviceRestart,getServiceStatus) have no JSDoc documenting parameters and optional-vs-required semantics.As per coding guidelines: "Add JSDoc comments for all public APIs and document optional versus required parameters."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/providers/P2pProvider.ts` around lines 1774 - 1909, Add JSDoc comments for each new public API in P2pProvider: getServiceTemplates, serviceStart, serviceStop, serviceExtend, serviceRestart, and getServiceStatus. Document the purpose of each method and clearly mark required versus optional parameters, especially chainId, signal, serviceId, and userData, matching the style used by the HttpProvider counterparts.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/services/providers/P2pProvider.ts`:
- Around line 1774-1909: Add JSDoc comments for each new public API in
P2pProvider: getServiceTemplates, serviceStart, serviceStop, serviceExtend,
serviceRestart, and getServiceStatus. Document the purpose of each method and
clearly mark required versus optional parameters, especially chainId, signal,
serviceId, and userData, matching the style used by the HttpProvider
counterparts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8705ef43-2cb2-405e-8a87-c0b0ca3ba259
📒 Files selected for processing (4)
.github/workflows/ci.ymlsrc/services/providers/BaseProvider.tssrc/services/providers/HttpProvider.tssrc/services/providers/P2pProvider.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/ci.yml
- src/services/providers/BaseProvider.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
package.json (1)
94-94: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
@oceanprotocol/contracts@^2.9.0drops config keys expected byConfigHelper.ts
address.jsonis missingAccessListFactory,Escrow, andEnterpriseFeeCollectorfor most networks, so this bump will leaveConfigHelper.tswithundefinedSDK config fields on those chains. Keep a compatible contracts release or make the lookup optional before merging.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` at line 94, The dependency upgrade to `@oceanprotocol/contracts`@^2.9.0 removes configuration keys required by ConfigHelper.ts, causing undefined SDK fields on several networks. Either retain a contracts version that includes AccessListFactory, Escrow, and EnterpriseFeeCollector, or update ConfigHelper.ts to handle missing lookups safely before applying this dependency change.src/services/providers/P2pProvider.ts (1)
1863-1894: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPass
opSignal()as thesignalargument here
test/integration/Services.test.ts:321—serviceRestart(providerUrl, consumerAccount, serviceId, undefined, opSignal())leavessignalundefined and binds the timeout todockerCmd. Useundefined, undefined, opSignal()or an options object to avoid fragile positional ordering.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/providers/P2pProvider.ts` around lines 1863 - 1894, Update the serviceRestart call sites, especially the integration test invocation, so opSignal() is passed as the signal parameter rather than being bound to dockerCmd; provide explicit undefined placeholders for userData and dockerCmd (and dockerEntrypoint as needed), or refactor to an options object while preserving the serviceRestart signature.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@package.json`:
- Line 94: The dependency upgrade to `@oceanprotocol/contracts`@^2.9.0 removes
configuration keys required by ConfigHelper.ts, causing undefined SDK fields on
several networks. Either retain a contracts version that includes
AccessListFactory, Escrow, and EnterpriseFeeCollector, or update ConfigHelper.ts
to handle missing lookups safely before applying this dependency change.
In `@src/services/providers/P2pProvider.ts`:
- Around line 1863-1894: Update the serviceRestart call sites, especially the
integration test invocation, so opSignal() is passed as the signal parameter
rather than being bound to dockerCmd; provide explicit undefined placeholders
for userData and dockerCmd (and dockerEntrypoint as needed), or refactor to an
options object while preserving the serviceRestart signature.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f9d84d3a-b540-49f0-a2a1-195bcf04e1fa
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
.github/workflows/ci.ymlCHANGELOG.mdpackage.jsonsrc/@types/Provider.tssrc/services/providers/BaseProvider.tssrc/services/providers/HttpProvider.tssrc/services/providers/P2pProvider.ts
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/ci.yml
- src/@types/Provider.ts
- src/services/providers/BaseProvider.ts
- src/services/providers/HttpProvider.ts
* notify incentive-backend when service started * add the exposedports, resources and updatedsince * fix lint and notify multiple services if the case --------- Co-authored-by: andreip136 <129227833+andreip136@users.noreply.github.com>
* edit started model * change to params * catch response ok
3 participants
This is base for the next major release, v9.0
Summary by CodeRabbit
@oceanprotocol/contractsto v2.9.0.