Skip to content
This repository has been archived by the owner on Jun 11, 2024. It is now read-only.

Update block schema - Closes #6739 #6765

Merged

Conversation

shuse2
Copy link
Collaborator

@shuse2 shuse2 commented Sep 13, 2021

What was the problem?

This PR resolves #6739

How was it solved?

  • Update block schema with new definition
  • Update lisk-chain to store assets
  • Update state machine context to have assets
  • Update consensus to use block header property directly

How was it tested?

  • Update unit tests

@shuse2 shuse2 self-assigned this Sep 13, 2021
@shuse2 shuse2 linked an issue Sep 13, 2021 that may be closed by this pull request
@shuse2 shuse2 requested review from mitsuaki-u and removed request for ishantiw September 13, 2021 15:49
if (typeof header !== 'object') {
throw new Error('Invalid block format. header must be an object.');
}
if (!Array.isArray(payload)) {
throw new Error('Invalid block format. payload must be an array.');
}
if (!Array.isArray(assets)) {
throw new Error('Invalid block format. assets must be an array.');
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
throw new Error('Invalid block format. assets must be an array.');
throw new Error('Invalid block format. assets must be an array.');
Suggested change
throw new Error('Invalid block format. assets must be an array.');
throw new Error('Invalid block format. "assets" must be an array.');

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 think this is following the style of other error message?

Copy link
Contributor

Choose a reason for hiding this comment

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

I saw further below the property name was in double quotes but I'm ok with either way.

elements/lisk-chain/src/block_header.ts Outdated Show resolved Hide resolved
},
};

export const blockAssetSchema = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would be more appropriate if we follow the naming conventions in the LIP.
The LIP defines new block property assets, and since we have blockHeaderSchema for the property header, hence blockAssetsSchema would be inline.

Suggested change
export const blockAssetSchema = {
export const blockAssetsSchema = {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

asset is not part of the block header, and we need to decode asset one by one. Notice block schema has byte array for assets

elements/lisk-chain/test/unit/block_assets.spec.ts Outdated Show resolved Hide resolved
elements/lisk-chain/test/unit/block_assets.spec.ts Outdated Show resolved Hide resolved
elements/lisk-chain/test/unit/block_assets.spec.ts Outdated Show resolved Hide resolved
elements/lisk-chain/test/unit/block_assets.spec.ts Outdated Show resolved Hide resolved
public constructor(
public readonly header: BlockHeader,
public readonly payload: Transaction[],
public readonly assets: BlockAssets,
Copy link
Contributor

Choose a reason for hiding this comment

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

A bit unsure here if it can be redefined to be something like BlockAsset[]. In the LIP it says assets is an array of objects and also since payload is an array of objects too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

other properties are also class instance, and we could create BlockAsset class, but we want to have blockAssets.get and blockAssets.set. Therefore it's better to be BlockAssets class

@shuse2 shuse2 requested a review from Incede September 15, 2021 07:36
@shuse2 shuse2 merged commit 43c20f3 into feature/6554-improve-framework-architecture Sep 15, 2021
@shuse2 shuse2 deleted the 6739-update_block_schema branch September 15, 2021 08:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update block schema and usages
3 participants