Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/776 circuit inputs parser #778

Merged
merged 12 commits into from
Feb 5, 2024
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,259 @@
import { DeepMockProxy, mockDeep } from 'jest-mock-extended';
import { LoggingService } from '../../../../../shared/logging/logging.service';
import { CircuitInputsParserService } from './circuitInputParser.service';

let cips: CircuitInputsParserService;
const loggingServiceMock: DeepMockProxy<LoggingService> =
mockDeep<LoggingService>();

beforeAll(async () => {
cips = new CircuitInputsParserService(loggingServiceMock);
});

describe('CircuitInputsParserService', () => {
it('Should return null if empty CircuitInputsMapping passed in', () => {
// Arrange
const payload = 'test';
const schema = {} as CircuitInputsMapping;

// Act
const circuitInputs = cips.applyMappingToJSONPayload(payload, schema);

// Assert
expect(circuitInputs).toBeNull();
});

it('Should generate a single circuit input param with charcode sum based on a single string param at root level', () => {
// Arrange
const payload = `{
"supplierInvoiceID": "INV123"
}`;

const schema = {
mapping: [
{
circuitInput: 'dascircuitinput',
description: 'desc',
payloadJsonPath: 'supplierInvoiceID',
dataType: 'string',
} as CircuitInputMapping,
],
} as CircuitInputsMapping;

// Act
const circuitInputs = cips.applyMappingToJSONPayload(payload, schema);

// Assert
expect(circuitInputs).toStrictEqual({ dascircuitinput: 387 });
});

it('Should generate a single circuit input param with charcode sum based on a default value for a missing string param at root level', () => {
// Arrange
const payload = `{
"somethingElse": ""
}`;

const defaultParamValue = '555333';

const schema = {
mapping: [
{
circuitInput: 'dascircuitinput',
description: 'desc',
payloadJsonPath: 'supplierInvoiceID',
dataType: 'string',
defaultValue: defaultParamValue,
} as CircuitInputMapping,
],
} as CircuitInputsMapping;

// Act
const circuitInputs = cips.applyMappingToJSONPayload(payload, schema);

// Assert
expect(circuitInputs).toStrictEqual({ dascircuitinput: 312 });
});

it('Should return null based on a missing default value for a missing string param at root level', () => {
// Arrange
const payload = `{
"somethingElse": ""
}`;

const schema = {
mapping: [
{
circuitInput: 'dascircuitinput',
description: 'desc',
payloadJsonPath: 'supplierInvoiceID',
dataType: 'string',
} as CircuitInputMapping,
],
} as CircuitInputsMapping;

// Act
const circuitInputs = cips.applyMappingToJSONPayload(payload, schema);

// Assert
expect(circuitInputs).toBeNull();
});

it('Should generate a single circuit input param with charcode sum based on a single string param at root + 1 level', () => {
// Arrange
const payload = `{
"supplierInvoice": {
"ID": "INV123"
}
}`;

const schema = {
mapping: [
{
circuitInput: 'dascircuitinput',
description: 'desc',
payloadJsonPath: 'supplierInvoice.ID',
dataType: 'string',
} as CircuitInputMapping,
],
} as CircuitInputsMapping;

// Act
const circuitInputs = cips.applyMappingToJSONPayload(payload, schema);

// Assert
expect(circuitInputs).toStrictEqual({ dascircuitinput: 387 });
});

it('Should generate a single circuit input param based on a single integer param at root level', () => {
// Arrange
const payload = `{
"supplierInvoiceID": 123
}`;

const schema = {
mapping: [
{
circuitInput: 'dascircuitinput',
description: 'desc',
payloadJsonPath: 'supplierInvoiceID',
dataType: 'integer',
} as CircuitInputMapping,
],
} as CircuitInputsMapping;

// Act
const circuitInputs = cips.applyMappingToJSONPayload(payload, schema);

// Assert
expect(circuitInputs).toStrictEqual({ dascircuitinput: 123 });
});

it('Should generate a single circuit input param based on a default value for a missing integer param at root level', () => {
// Arrange
const payload = `{
"somethingElse": ""
}`;

const defaultValue = 555333;

const schema = {
mapping: [
{
circuitInput: 'dascircuitinput',
description: 'desc',
payloadJsonPath: 'supplierInvoiceID',
dataType: 'integer',
defaultValue: defaultValue,
} as CircuitInputMapping,
],
} as CircuitInputsMapping;

// Act
const circuitInputs = cips.applyMappingToJSONPayload(payload, schema);

// Assert
expect(circuitInputs).toStrictEqual({ dascircuitinput: defaultValue });
});

it('Should return null based on a missing default value for a missing integer param at root level', () => {
// Arrange
const payload = `{
"somethingElse": ""
}`;

const schema = {
mapping: [
{
circuitInput: 'dascircuitinput',
description: 'desc',
payloadJsonPath: 'supplierInvoiceID',
dataType: 'integer',
} as CircuitInputMapping,
],
} as CircuitInputsMapping;

// Act
const circuitInputs = cips.applyMappingToJSONPayload(payload, schema);

// Assert
expect(circuitInputs).toBeNull();
});

it('Should generate a single circuit input param based on a single integer array param at root level', () => {
// Arrange
const payload = `{
"supplierInvoiceIDs": [
123,
321,
454
]
}`;

const schema = {
mapping: [
{
circuitInput: 'dascircuitinput',
description: 'desc',
payloadJsonPath: 'supplierInvoiceIDs',
dataType: 'array',
arrayType: 'integer',
} as CircuitInputMapping,
],
} as CircuitInputsMapping;

// Act
const circuitInputs = cips.applyMappingToJSONPayload(payload, schema);

// Assert
expect(circuitInputs).toStrictEqual({ dascircuitinput: [123, 321, 454] });
});

it('Should generate a single circuit input param with charcode sums based on a single string array param at root level', () => {
// Arrange
const payload = `{
"supplierInvoiceIDs": [
"INV123",
"INV124",
"INV125"
]
}`;

const schema = {
mapping: [
{
circuitInput: 'dascircuitinput',
description: 'desc',
payloadJsonPath: 'supplierInvoiceIDs',
dataType: 'array',
arrayType: 'string',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree. the circuitInput for example will be an object that has a schema itself depending on the type of input parameters.

} as CircuitInputMapping,
],
} as CircuitInputsMapping;

// Act
const circuitInputs = cips.applyMappingToJSONPayload(payload, schema);

// Assert
expect(circuitInputs).toStrictEqual({ dascircuitinput: [387, 388, 389] });
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
import { Injectable } from '@nestjs/common';
import { LoggingService } from '../../../../../shared/logging/logging.service';

@Injectable()
export class CircuitInputsParserService {
constructor(private readonly logger: LoggingService) {}

public applyMappingToJSONPayload(payload: string, cim: CircuitInputsMapping) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreement is not to use the library per the last SRI sync call. Will address other comments if any and submit for rereview

const result: any = {};

try {
const jsonPayload = JSON.parse(payload);

for (let mapping of cim.mapping) {

Check failure on line 14 in examples/bri-3/src/bri/zeroKnowledgeProof/services/circuit/circuitInputsParser/circuitInputParser.service.ts

View workflow job for this annotation

GitHub Actions / ci (ubuntu-latest, 16.17.0)

'mapping' is never reassigned. Use 'const' instead
const value = this.getJsonValueByPath(
jsonPayload,
mapping.payloadJsonPath,
);

if (!value && !mapping.defaultValue) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we check value === undefined, if value can be empty string? since we are checking ===undefined in method itself

this.logger.logError(
`Missing value and default value for mapping ${cim.mapping} while mapping circuit inputs for payload ${payload}`,
);
return null;
}

switch (mapping.dataType) {
case 'string':
result[mapping.circuitInput] = value
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this be simplified, like result... = this.calc(value || default)

? this.calculateStringCharCodeSum(value)
: this.calculateStringCharCodeSum(mapping.defaultValue);
break;

case 'integer':
result[mapping.circuitInput] = value ?? mapping.defaultValue;
break;

case 'array':
if (mapping.arrayType === 'string') {
result[mapping.circuitInput] = value
? value.map((val) => this.calculateStringCharCodeSum(val))
: mapping.defaultValue.map((val) =>
this.calculateStringCharCodeSum(val),
);
}

if (mapping.arrayType === 'integer') {
result[mapping.circuitInput] = value ?? mapping.defaultValue;
}

if (mapping.arrayType === 'object') {
// TODO
}
break;
default:
this.logger.logError(
`Unknown datatype '${mapping.dataType}' while mapping circuit inputs for payload ${payload}`,
);
return null;
}
}
} catch (error) {
this.logger.logError(
`Error '${error}' while mapping circuit inputs for payload ${payload}`,
);
return null;
}

return result;
}

private getJsonValueByPath(json: any, path: string) {
const parts = path.split('.');
let currentValue = json;

for (const part of parts) {
if (currentValue[part] === undefined) {
return undefined;
}
currentValue = currentValue[part];
}

return currentValue;
}

private calculateStringCharCodeSum(text: string): number {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i like the good old for loop here and there

let sum = 0;

for (let i = 0; i < text.length; i++) {
sum += text.charCodeAt(i);
}

return sum;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
type CircuitInputsMapping = {
mapping: CircuitInputMapping[];
};

type CircuitInputMapping = {
circuitInput: string;
description: string;
payloadJsonPath: string;
dataType: string;
defaultValue?: any;
arrayType?: string;
};
Loading