add updateServiceTimeout and edit ddo updates

[alexcos20]

Breaking changes in editMetadata definition !

closes #536 and #525

Please make sure that you call ocean.OnChainMetadataCache.update after using the helpers, otherwise your asset it's not going to be updated on chain

[alexcos20]

waiting for #542 , so I can rebase, that's the only test that is failing no.

[kremalicious]

all these null returns could be done in one if statement in line 293 instead of sprinkling the conditions throughout multiple lines:

if (
  !ddo ||
  ddo.service[serviceIndex].type !== 'compute' ||
  typeof ddo.service[serviceIndex] === 'undefined'
)
  return null

[alexcos20]

actually, the logic is:

• try to get the ddo if we only have the DID (if we have the DDO already, avoid extra call to aqua)
• if serviceIndex == -1 -> try to search for compute service
• else check if serviceIndex is a valid compute service (maybe it's not)

[kremalicious]

and why does this method here not do that for me?

[kremalicious]

got it, because of the method split-up. I know conceptually updating timeout is not updating the metadata service but maybe it makes sense to handle that within editMetadata too since from user perspective it's also just metadata.

So EditableMetadata could have:

timeout?: {
  serviceIndex: number // could also ask for `serviceName` which would be a bit more user friendly
  value: number
}

and if passed we handle that in that method (where updateServiceTimeout can be left as individual method, but called here), allowing us to also call ocean.OnChainMetadataCache.update at the end. Thoughts?

[alexcos20]

I'm afraid that we are going to end up with a huge function that does all.. also, don't forget that we have compute updates as well, and who knows what kind of updates are we going to have in the future..
That's why I thought that it makes sense to have update helpers (functions that are going to update parts of the ddo) and once you are done, you only have 1 call that updates the ddo on chain.
What if you want to change computePrivacy, timeout and asset's name ?

[kremalicious]

guess I'm arguing for a helper method doing all then, calling the sub-functions. Since we know which fields are editable we should provide a helper so devs do not need to figure out for themselves which parts of DDO are editable and then which ones require which method to update.

This helper method handling all editable fields could simply call the individual sub-functions. Then it also stays flexible, advanced users can use the individual methods, but most will use the main helper method with a bunch of optional params. And then also stay consistent in the naming to group the methods, like:

• editMetadata()
  • editMetadataFields()
  • editTimeout()
  • editComputePrivacy()

If we decide to keep as is then at least solve this mixup of edit & update and stick to one

[codeclimate]

Code Climate has analyzed commit 29ebf76 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 90.7% (50% is the threshold).

This pull request will bring the total coverage in the repository to 78.6% (1.0% change).

View more on Code Climate.

[mihaisc]

So after we call editMetadata and/or editServiceTimeout we should call ocean.OnChainMetadataCache.update ? If yes, why is the update in OnChainMetadataCache ? It has nothing to do with the cache. Considering where it is, this function should update the cache.

[alexcos20]

actually we have another class for that, it's called MetadataCache :)
@mihaisc For consistency, and if @kremalicious agrees too, I would rename that to OnChainMetadata

Sounds good?