Feature/escrow_payments by alexcos20 · Pull Request #828 · oceanprotocol/ocean-node

alexcos20 · 2025-01-30T09:19:18Z

Changes proposed in this PR:

remove OPF_k8
add escrow payments
update interfaces for paid Compute Jobs (BREAKING CHANGES)

How to use:

start barge with

export CONTRACTS_VERSION=escrow
./start_ocean.sh --no-aquarius --no-provider --no-dashboard --with-typesense

Nice TODOs (can be done later):

more tests for lock/claim
docs

… SC)

…trings as well

mariacarmina

I left some comments, still need to review feesHandler & and test the end2end flow, because it contains many breaking changes.

mariacarmina · 2025-02-06T13:05:32Z

+    const signer = blockchain.getSigner()
+    const contract = await this.getContract(chainId, signer)
+    try {
+      return await contract.getAuthorizations(token, payer, payee)


Those functions, at least most of them are implemented here, when a new ocean.js version will be released, I would say that contract object creation and calling contract function to be replaced with the methods in ocean.js. In ocean.js I can also add the additional checks that you managed to write within ocean-node.

what's the relation? ocean-node does not use ocean.js

I don't remember why we decided not to use ocean.js in ocean node? I can see that it would avoid duplication if we imported these functions from ocean.js rather than maintaining them in both places

@alexcos20 it is helpful to use the lib when we interact with smart contracts, at least that's a suggestion, we also have ethers in ocean-node to interact with smart contracts. I've just seen that some similar functions from ocean-node for escrow payments (and not only) that can be imported from ocean.js

well, i actually disagree on this.. IMO is better to NOT use the SDK on the nodes, AS IS... otherwise we might have to keep SDK updated (and release a new version) every time we do a big change on the Nodes... Every contract is different and has different functions, we don't need the SDK to interact with a smart contract..

we have ethers for that.. If we're gonna use the SDK on the Nodes... we might have issues, as we need to keep always 4 different pieces of software in sync; the SDK, the CLI (which we also use for the system tests) , the Smart Contracts and the Nodes, .... and we certainly don't need all the extra functionality available on the SDK whe we just need to call a smart class function..

Are we actually discussing introducing the SDK dependency because of one single function that creates a smart contract instance??? this.getContract(..) ? (cause all the rest is the same but with the overhead of creating further classes)

It's an interesting discussion, I think it depends if it could be useful in multiple places and not just one instance. I don't quite see it the same as you @paulo-ocean

otherwise we might have to keep SDK updated (and release a new version) every time we do a big change on the Nodes

That shouldn't be the case, if the SDK covers all of the functionality in the contracts the only thing that you would change would be the arguments that you are passing to the SDK functions, no need for a new release. The new release of the SDK would just be when the contracts are updated.

we might have issues, as we need to keep always 4 different pieces of software in sync; the SDK, the CLI (which we also use for the system tests) , the Smart Contracts and the Nodes

The SDK should always be up-to-date with the contracts anyway, otherwise, what's the point in having it?

The CLI already uses ocean.js to interact with the contracts, so there is no extra work there.

So it's only a question of if the nodes could benefit from using the SDK. And it shouldn't create extra work, if there is a function in the SDK that is duplicated here it reduces the amount of code that we need to maintain if it is imported.

Are we actually discussing introducing the SDK dependency because of one single function that creates a smart contract instance???

I agree with this - it's not worth it for one function call, only if it's going to be used in multiple places.

Its ok that we have different views :-)
I personally don't see any need for SDK on the nodes, and i think it will bring more way more issues than solutions.
Afaik, the SDK is mainly a library to abstract indexer, provider, aquarius, etc.. deal with configurations, URLS, etc..
This is all part of the nodes monolithic infrastructure... don´t see how its helpful here

Regarding the smart contracts in particular, I don't think we need an SDK to encapsulate every smart contract functionality into a typescript class, just to eventually call a smart contract function. And i definitely don't see that as a code duplication...

its just an enclosing function to call the smart contract level function.. If we use the SDK for this particular case its exactly the same thing... except that we have an extra Instance in memory, and we call the encapsulating function that actually does the smart contract call... And if we need a bit more logic on the node side, what do we do then? We update the SDK? Because if we don't we probably already lost the sdk "benefit" here anyway

Anyway, my 2 cents... doesn't mean i'm right or wrong

Why we would integrate ocean-js in ocean-node? For 3 (three) functions in a smartcontract? Which, for now, don't even overlap? (ie: they do not exist in ocean.js)

jamiehewitt15 · 2025-02-13T12:35:32Z

We will need some documentation on how the escrow payments work, is that going to come in a separate PR?

jamiehewitt15 · 2025-02-13T12:40:04Z

-        console.log('timeNow: ' + timeNow + ' , Expiry: ' + job.expireTimestamp)
-        if (timeNow > job.expireTimestamp || job.stopRequested) {
+        const expiry = parseFloat(job.algoStartTimestamp) + job.maxJobDuration
+        console.log('timeNow: ' + timeNow + ' , Expiry: ' + expiry)


Why are you using console.log rather than our CustomNodeLogger?

jamiehewitt15 · 2025-02-13T12:43:26Z

+        return {
+          stream: null,
+          status: {
+            httpStatus: 500,
+            error: 'Invalid C2D Environment'
+          }
+        }
+      }
+      if (engine === null) {
+        return {
+          stream: null,
+          status: {
+            httpStatus: 500,
+            error: 'Invalid C2D Environment'
+          }
+        }
+      }
+      try {
+        env = await engine.getComputeEnvironment(task.payment.chainId, task.environment)
+        if (!env) {
+          return {
+            stream: null,
+            status: {
+              httpStatus: 500,
+              error: 'Invalid C2D Environment'
+            }
+          }
+        }
+


It would be good if each of these 500 responses had a different error message so we know exactly where it is coming from and which check failed.

jamiehewitt15 · 2025-02-13T12:46:30Z

+              // TO DO:  Edge case when this asset is served by a remote provider.
+              // We should connect to that provider and get the fee


Looks like this was left here from before your changes, is it still the plan? If so we need an issue for it

jamiehewitt15 · 2025-02-13T12:50:00Z

+    try {
+      const funds = await contract.getUserFunds(payer, token)
+      return funds.available
+    } catch (e) {


Is there any reason why you didn't log the errors in any of the functions in this file? It's going to make it hard to debug when something isn't working...

paulo-ocean · 2025-02-13T12:59:15Z

+
  // eslint-disable-next-line no-useless-constructor
  private constructor(
+    private config: OceanNodeConfig,


why do we need to add the config? we already did the same change a couple of times in the past.. but the config is something that can change over time (very frequent on the tests).. The initial idea was to create a node Instance based on the initial config
Afaik, its only usage here is to "decide" if we should also add both C2D engines and Escrow components... so why not add those on the constructor (like we do for other optional components, like; P2P, DB, Indexer, etc ?)... like:

public static getInstance( // config?: OceanNodeConfig, db?: Database, node?: OceanP2P, provider?: OceanProvider, indexer?: OceanIndexer, c2dEngines?: C2DEngines, escrow?: Escrow ): OceanNode {

this ways its the same approach for all components
we also have this piece in the constructor

if (this.config) { this.escrow = new Escrow( this.config.supportedNetworks, this.config.claimDurationTimeout ) }

but the config is not optional and there is always a configuration object when we start a node.. so the escrow creation is kind of mandatory anyway

paulo-ocean · 2025-02-17T09:12:08Z


+## Payments
+
+- `ESCROW_CLAIM_TIMEOUT`: Amount of time reserved to claim a escrow payment, in seconds. Defaults to `600`. Example: `600`


i think we should also add the variable reference to constants/ENVIRONMENT_VARIABLES, env.example and scripts/ocean-node-quickstart.sh

Co-authored-by: Jamie Hewitt <jamie@oceanprotocol.com>

mariacarmina

At initializeCompute https://github.com/oceanprotocol/ocean-node/pull/828/files#diff-71521de5df77d93ae85ff75f78504f70beec812c2e776bf25e990a0d5d4c37b8R284 we do not create any task object specific for compute initialize, I'd suggest to have some checks for the following keys: 'payment', 'consumerAddress', 'environment' -> then we can update also the API.md for compute routes to be easier to follow the parameters.

…to feature/escrow_payments

mariacarmina

One question from my side, otherwise it's good to start with this baseline and not keeping in review for too much time.

mariacarmina · 2025-04-14T12:55:17Z

              DOCKERHUB_PASSWORD: ${{ secrets.DOCKERHUB_PASSWORD }}
      - name: Run Barge
        working-directory: ${{ github.workspace }}/barge
+        env:


Can we can update barge with default version to escrow? Or after we have this escrow integration in place and see if works on all impacted repos?

I will take care of this

alexcos20 and others added 30 commits December 3, 2024 14:50

start work on docker c2d

4767ae9

more on db

b355b2d

fix c2d database

048c68a

BREAKING: refactor ComputeAsset and ComputeStartCommand interfaces

f866a10

add freeCompute handler

fd1f279

working POC of Docker compute

92c0fd2

small fixes

81e8ac5

update deps

a69a07c

update ignores

57ccbed

fix getComputeJobResult and getComputeJobStatus

2a7ac31

wip: add sql lite db

5ad1953

more changes + refactor internal blob/body

6d71253

add get running jobs

61459f6

add logging

d6c74e9

wip: adding cron to delete expired storage and expired jobs

90814f1

add test for getting expied and clean

dab2a02

refactor deletion, add .env properties

1a27b2f

small refactor

b34319d

wip: add env vars for docket compute envs

04064f2

wip: add env vars for docket compute envs

a83e68f

add config options

ad605a1

basic cd2 free compute flow

d61967d

add some comments

d562d90

add optional signature, for template 4 (need to get files object from…

72bf88e

… SC)

add consume data/signature for start/init c2d job

e0789a4

add some comments

8b6fd45

add c2d docker options on quick start script

a834239

improve script for envs, improve config function to deal with empty s…

8427f6e

…trings as well

add unit test for free docker env

2443ab4

add integration test free docker start compute

9aa6b6d

alexcos20 requested a review from paulo-ocean as a code owner February 5, 2025 06:55

mariacarmina reviewed Feb 6, 2025

View reviewed changes

jamiehewitt15 reviewed Feb 13, 2025

View reviewed changes

Comment thread src/components/database/sqliteCompute.ts Outdated

paulo-ocean reviewed Feb 13, 2025

View reviewed changes

paulo-ocean reviewed Feb 17, 2025

View reviewed changes

Comment thread src/@types/C2D/C2D.ts

Update src/components/database/sqliteCompute.ts

56baf45

Co-authored-by: Jamie Hewitt <jamie@oceanprotocol.com>

alexcos20 mentioned this pull request Feb 24, 2025

Check initializeCompute and getExactComputeEnv (chainId logic) #848

Closed

Base automatically changed from feature/docker_refactor to feature/c2d_docker February 25, 2025 07:32

mariacarmina reviewed Mar 7, 2025

View reviewed changes

Base automatically changed from feature/c2d_docker to main March 21, 2025 13:44

alexcos20 requested a review from giurgiur99 as a code owner March 21, 2025 13:44

giancu4 assigned mariacarmina Mar 24, 2025

giancu4 added the Status: Blocked label Mar 24, 2025

alexcos20 added 3 commits April 1, 2025 08:58

Merge branch 'main' into feature/escrow_payments

e386331

use latest escrow

0caf26c

Merge branch 'main' of https://github.com/oceanprotocol/ocean-node in…

64b36db

…to feature/escrow_payments

alexcos20 removed the Status: Blocked label Apr 1, 2025

alexcos20 assigned alexcos20 and unassigned mariacarmina Apr 1, 2025

fix timeouts

df6e0fb

giurgiur99 approved these changes Apr 14, 2025

View reviewed changes

mariacarmina approved these changes Apr 14, 2025

View reviewed changes

alexcos20 merged commit e18ddc8 into main Apr 15, 2025
13 checks passed

alexcos20 deleted the feature/escrow_payments branch April 15, 2025 03:40

		// TO DO: Edge case when this asset is served by a remote provider.
		// We should connect to that provider and get the fee


		## Payments

		- `ESCROW_CLAIM_TIMEOUT`: Amount of time reserved to claim a escrow payment, in seconds. Defaults to `600`. Example: `600`

Conversation

alexcos20 commented Jan 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mariacarmina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulo-ocean Feb 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulo-ocean Feb 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamiehewitt15 commented Feb 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

paulo-ocean Feb 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mariacarmina left a comment

Choose a reason for hiding this comment

Uh oh!

mariacarmina left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

alexcos20 commented Jan 30, 2025 •

edited

Loading

paulo-ocean Feb 17, 2025 •

edited

Loading

paulo-ocean Feb 17, 2025 •

edited

Loading

paulo-ocean Feb 13, 2025 •

edited

Loading