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

Repeated or duplicated properties in schemas #287

Closed
sitetester opened this issue Feb 27, 2023 · 4 comments · Fixed by #299
Closed

Repeated or duplicated properties in schemas #287

sitetester opened this issue Feb 27, 2023 · 4 comments · Fixed by #299
Assignees

Comments

@sitetester
Copy link
Contributor

sitetester commented Feb 27, 2023

https://github.com/LiskHQ/lips/blob/main/proposals/lip-0061.md#schema

unsignedCertificateSchema = {
    "type": "object",
    "required": [
        "blockID",
        "height",
        "timestamp",
        "stateRoot",
        "validatorsHash"
    ],
    "properties": {
        "blockID": {
            "dataType": "bytes",
            "length": HASH_LENGTH,
            "fieldNumber": 1
        },
        "height": {
            "dataType": "uint32",
            "fieldNumber": 2
        },
        "timestamp": {
            "dataType": "uint32",
            "fieldNumber": 3
        },
        "stateRoot": {
            "dataType": "bytes",
            "length": HASH_LENGTH,
            "fieldNumber": 4
        },
        "validatorsHash": {
            "dataType": "bytes",
            "length": HASH_LENGTH,
            "fieldNumber": 5
        }
    }
}
certificateSchema = {
    "type": "object",
    "required": [
        "blockID",
        "height",
        "timestamp",
        "stateRoot",
        "validatorsHash",
        "aggregationBits",
        "signature"
    ],
    "properties": {
        "blockID": {
            "dataType": "bytes",
            "length": HASH_LENGTH,
            "fieldNumber": 1
        },
        "height": {
            "dataType": "uint32",
            "fieldNumber": 2
        },
        "timestamp": {
            "dataType": "uint32",
            "fieldNumber": 3
        },
        "stateRoot": {
            "dataType": "bytes",
            "length": HASH_LENGTH,
            "fieldNumber": 4
        },
        "validatorsHash": {
            "dataType": "bytes",
            "length": HASH_LENGTH,
            "fieldNumber": 5
        },
        "aggregationBits": {
            "dataType": "bytes",
            "fieldNumber": 6
        },
        "signature": {
            "dataType": "bytes",
            "length": BLS_SIGNATURE_LENGTH,
            "fieldNumber": 7
        }
    }
}

Just like in LIP 45, we used ... notation to merge external schema, we can do the same here to merge repeated properties from unsignedCertificateSchema

One example:
https://github.com/LiskHQ/lips/blob/main/proposals/lip-0045.md#data-3

ccmProcessedDataSchema = {
    "type": "object",
    "required": ["ccm", "result", "code"],
    "properties": {
        "ccm": {
            ...crossChainMessageSchema,
            "fieldNumber": 1
        },
        "result": {
            "dataType": "uint32",
            "fieldNumber": 2
        },
        "code": {
            "dataType": "uint32",
            "fieldNumber": 3
        }
    }
}

Also we can add this text below updated schema:

Here, the ... notation, borrowed from JavaScript ES6 data destructuring, indicates that the corresponding schema should be inserted in place, and it is just used for notational convenience.

Note: Above text is taken from https://github.com/LiskHQ/lips/blob/main/proposals/lip-0045.md#genesis-assets-schema, which is written under the schema.

@janhack
Copy link
Contributor

janhack commented Feb 28, 2023

@sitetester I agree for nested objects, it is nice and clean to just use the ... notation. However, in this case here we need to add two elements to the "required" array and "properties" array and do not just define a nested sub-schema. Therefore, I am not sure that it would be that clean in this case.

@janhack janhack self-assigned this Feb 28, 2023
@sitetester
Copy link
Contributor Author

sitetester commented Mar 3, 2023

Hi, thank you for asking question & clarifying ambiguity!
Yes, we can deal with repeated properties in required section also. In fact, it's already managed in current code.

export const certificateSchema = {
	type: 'object',
	required: [...unsignedCertificateSchema.required, 'aggregationBits', 'signature'],
	properties: {
		...unsignedCertificateSchema.properties,
		aggregationBits: {
			dataType: 'bytes',
			fieldNumber: 6,
		},
		signature: {
			dataType: 'bytes',
			minLength: BLS_SIGNATURE_LENGTH,
			maxLength: BLS_SIGNATURE_LENGTH,
			fieldNumber: 7,
		},
	},
};

It's all good like this. We should avoid repetition as much as possible.

@janhack
Copy link
Contributor

janhack commented Mar 6, 2023

Nice, thanks for the reference. I will update the LIP accordingly.

@janhack
Copy link
Contributor

janhack commented Mar 9, 2023

I think we could do the same change for the block header schema https://github.com/LiskHQ/lips/blob/main/proposals/lip-0055.md#unsigned-block-header-json-schema. I generalized the issue name accordingly.

@janhack janhack changed the title [LIP61] certificateSchema has repeated/duplicated properties from unsignedCertificateSchema Repeated or duplicated properties in schemas Mar 9, 2023
@janhack janhack assigned AnanyaSshri and unassigned janhack Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants