Feat/776 circuit inputs parser #778
Conversation
…o assert error handling
ognjenkurtic
requested review from
skarred14,
Therecanbeonlyone1969,
Kasshern,
Ybittan,
biscuitdey and
skosito
as code owners
| return currentValue; | ||
| } | ||
|
|
||
| private calculateStringCharCodeSum(text: string): number { |
There was a problem hiding this comment.
maybe this method can be more readable with usage of .map and things like that? text can be transformed into array using .split method and then .map can be used or something like that
but not super important
There was a problem hiding this comment.
i like the good old for loop here and there
| mapping.payloadJsonPath, | ||
| ); | ||
|
|
||
| if (!value && !mapping.defaultValue) { |
There was a problem hiding this comment.
should we check value === undefined, if value can be empty string? since we are checking ===undefined in method itself
|
|
||
| switch (mapping.dataType) { | ||
| case 'string': | ||
| result[mapping.circuitInput] = value |
There was a problem hiding this comment.
can this be simplified, like result... = this.calc(value || default)
| description: 'desc', | ||
| payloadJsonPath: 'supplierInvoiceIDs', | ||
| dataType: 'array', | ||
| arrayType: 'string', |
There was a problem hiding this comment.
do we need both array and arrayType? can arrayType be calculated using typeof array[0]? can typeof value be used in other places too to simplify mapping logic and remove some fields here?
i feel more validation of this object is needed, for example if dataType is not array, we should not specifiy arrayType and so on, but it seems like some simplification can be done first
There was a problem hiding this comment.
We want the participants to strictly define the types that are expected in the payload, so that we can do a type check before we even try to invoke the circuit. there is still definitely room for improvement, both for simplification and additional validation, but let's do it in small iterations as we move forward.
There was a problem hiding this comment.
agree. the circuitInput for example will be an object that has a schema itself depending on the type of input parameters.
…cuit input is provided in the circuit input parser; Simplify calculateStringCharCodeSum call in case of string type
| export class CircuitInputsParserService { | ||
| constructor(private readonly logger: LoggingService) {} | ||
|
|
||
| public applyMappingToJSONPayload(payload: string, cim: CircuitInputsMapping) { |
There was a problem hiding this comment.
@ognjenkurtic don't we just need a json schema validator for typscript. Why not use something like this https://github.com/ThomasAribart/json-schema-to-ts
That way we just need to have a json schema file for the circuit stored in a path and then can validate the provided input against that. Is that not easier than explicity writing doen all the types etc.?
There was a problem hiding this comment.
@Therecanbeonlyone1969 The lib above is written for a different use-case: solving code duplication when you do static and runtime type checking.
From their readme:
Both objects carry similar if not exactly the same information. This is a code duplication that can annoy developers and introduce bugs if not properly maintained.
Our use-case is different than that. We are not only validating that the tx payload is of a specific type, we are also applying specific business rules (translations) based on the type definitions for specific circuit inputs. Introduction of a lib like this would not help as we would still need to act based on the type of the property in the payload.
I would vote we start with what i did. We have a set of unit tests we ll continue to extend, so we ll have good infra in place to refactor if\whenever the need arises.
There was a problem hiding this comment.
@ognjenkurtic I do not like what you did. How about we use this? ... https://github.com/grantila/suretype ... looks easy to implement and very flexible when the user provides the schema.
There was a problem hiding this comment.
Agreement is not to use the library per the last SRI sync call. Will address other comments if any and submit for rereview
|
@Therecanbeonlyone1969 all comments are addressed now. Please approve so that i can merge. |
Description
Introduces a first version of a circuit input parser that translates tx payload into circuit input using a predefined schema.
Adds a number of unit tests to cover the basic functionality
Related Issue
#776
Motivation and Context
Allows the BPI to support general use-cases when it comes to circuits. Workgroup admin would provide the compiled circuits together with the schema, which would allow the BPI to parse the tx payload and execute the circuits
How Has This Been Tested
Introduced circuitInputParser.service.spec.ts with 10 new unit tests, covering most of the new functionality
Screenshots (if appropriate)
Types of changes
Checklist