Substra/substra-backend

Composite traintuple require public attribute for out_trunk_model_permissions only to ignore it

Closed this issue · 6 comments

jmorel commented

This issue was first noticed by @maeldebon (see Substra/substra#112 and Substra/substra#113)

The composite traintuple serializer requires the out_trunk_model_permission to contain a "public" value but then ignores it and replaces it with False.

We should just not require the value at all.

samlesu commented

As the value is ignored by the backend, it should be removed from the serializer.

samlesu commented

Moreover, it would be better to force it to False in the chaincode directly (and remove this field in the interface between the chaincode and the backend)?

thibaultrobert commented

I completely agree with @samlesu, if it supposed to be always false it should be done in the chaincode.
The only quarrel I may have, it's that I don't remember it was explicitly supposed to be false for every trunk model. But since it's already the case and it's compatible with the current use of composite traintuple let's do it.

  • 👍1
jmorel commented

@thibaultrobert can I let you handle the chaincode part of it? I'll update the backend and the tests.

thibaultrobert commented

I completely agree with @samlesu, if it supposed to be always false it should be done in the chaincode.
Contrary to what I've stated before, we don't need to modify the chaincode because the current behavior is as follow:

  • If there is a public field the permission are either public or restricted to the listed nodes depending on it's value.
  • If there is no public field, it's considered false, therefore the permission is restricted.

The required tag on public field (like here) is useless, misleading and should be removed but otherwise, I don't recommend to add a specific validation to check that trunk models have restricted authorization. Not in the chaincode anyway.