enableRichSchemas feature flag - take 1 #1626
Conversation
|
Can one of the admins verify this patch? |
|
This pull request introduces 5 alerts when merging 435cf42 into 8c3649a - view on LGTM.com new alerts:
|
|
(ci) test this please |
|
Not sure but the DCO check seems to be repo owner related, right? |
@AlexJonsson, All Hyperledger contributions require author signoff on all commits. Details on how to amend your commit can be found here; https://github.com/hyperledger/indy-node/pull/1626/checks?check_run_id=1266975922 |
Signed-off-by: AlexJonsson <alexander.t.jonsson@gmail.com>
|
This pull request introduces 5 alerts when merging 2c3ba35 into 8c3649a - view on LGTM.com new alerts:
|
|
Hi All; the warnings thrown in the LGTM link presume that the indy_common.config.py module is not imported first before the other modules (transactions, constants). Unless someone can point out the order of import of these modules upon build, I would submit that these cyclic warnings are alright (or someone else please provide 2nd opinion). @esplinr |
There was a problem hiding this comment.
Thanks a lot for your PR! It is really very important for the system, but there are some comments. The codebase is complex and not transparent somewhere. For a merge this PR would be great to do follow items:
- remove direct feature flag imports and remove all feature flag checks (marked these places with comments)
- add flag check to static validation and read it through
indy_common.config_util getConfig(). If rich schemas are disabled, respond to the client with NACK. - add a test that by default rich schema transactions are not written to the ledger and the client receives NACK reply.
- in tests that fall to make the inclusion of this feature flag through the tconf fixture. An example of changing configs in tests:
If you have any questions, feel free to ask. I'll be glad to help.
| # Rich Schema | ||
| enableRichSchemas = False | ||
|
|
||
| if enableRichSchemas == True: |
There was a problem hiding this comment.
And as I understand in if constructions if a condition is already bool, we can just write:
if enableRichSchemas:
Regardless of the state of the flag, the presence of unused parameters in the configuration will not affect the entire behavior. In general, we would like to avoid conditions in the configuration whenever possible. Could you remove this if please?
| CONTEXT_VERSION = "version" | ||
| CONTEXT_CONTEXT = "context" | ||
| CONTEXT_FROM = "dest" | ||
| if enableRichSchemas == True: |
There was a problem hiding this comment.
Could you remove this condition please? It doesn't affect the system but complicates the logic.
And please don't use imports for getting config. We have indy_common.config_util getConfig() to collect configs from all modules, some times they may be overwritten.
| SET_CONTEXT = "200" | ||
| GET_CONTEXT = "300" | ||
| # Rich Schema | ||
| if enableRichSchemas == True: |
There was a problem hiding this comment.
Could you remove this condition please? It doesn't affect the system but complicates the logic.
And please don't use imports for getting config. We have indy_common.config_util getConfig() to collect configs from all modules, some times they may be overwritten.
| (CONTEXT_VERSION, VersionField(version_cls=ContextVersion)), | ||
| (ORIGIN, IdentifierField(optional=True)) | ||
| ) | ||
| if enableRichSchemas == True: |
There was a problem hiding this comment.
It can be dangerous for a client's requests parsing. Could you remove this condition please?
| (DATA, SetContextField()), | ||
| ) | ||
| # Rich Schema | ||
| if enableRichSchemas == True: |
There was a problem hiding this comment.
It can be dangerous for a client's requests parsing. Could you remove this condition please?
| @@ -445,7 +447,34 @@ class ClientGetAuthRuleOperation(MessageValidator): | |||
|
|
|||
|
|
|||
| class ClientOperationField(PClientOperationField): | |||
| _specific_operations = { | |||
| if enableRichSchemas == True: | |||
There was a problem hiding this comment.
It can be dangerous for a client's requests parsing. Could you remove this condition please?
| def __init__(self, version: str, **kwargs): | ||
| super().__init__(version, parts_num=(2, 3), **kwargs) | ||
| # Rich Schema | ||
| if enableRichSchemas == True: |
There was a problem hiding this comment.
It can be dangerous for a client's requests parsing. Could you remove this condition please?
| req = sdk_sign_and_submit_req(sdk_pool_handle, sdk_wallet_steward, set_context_txn_json) | ||
| rep = sdk_get_and_check_replies(looper, [req]) | ||
| return rep | ||
| if enableRichSchemas == True: |
There was a problem hiding this comment.
This condition is unnecessary and may cause some tests to fail. Could you remove this condition please?
|
Closed after meeting with 11/24 Indy contributors call |
Aside from pulling the config variable 'enableRichSchemas' into the various CONTEXT related functions, I wasn't sure how else to implement this feature flag. Looking forward to improving this over time. I haven't been able to get my development workbench up for indy-node (only been just able to set up a von network). Will work to figure that out better. Have a great weekend!