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

Update chain to use lisk-codec with new schema - Closes #5264 #5367

Merged
merged 11 commits into from
Jun 2, 2020

Conversation

shuse2
Copy link
Collaborator

@shuse2 shuse2 commented May 28, 2020

What was the problem?

This PR resolves #5264

How was it solved?

  • Update to new account, block, blockheader schema
  • Update reference within chain

How was it tested?

  • All tests are updated

@shuse2 shuse2 self-assigned this May 28, 2020
@shuse2 shuse2 linked an issue May 28, 2020 that may be closed by this pull request
@ManuGowda ManuGowda force-pushed the 5270-encode-decode-transactions branch from 9886e56 to 08cb5b3 Compare May 28, 2020 14:29
@shuse2 shuse2 force-pushed the 5264-update_chain_codec branch from d8b8925 to 938539a Compare May 28, 2020 17:59
Base automatically changed from 5270-encode-decode-transactions to feature/update_to_use_codec May 29, 2020 08:23
@shuse2 shuse2 force-pushed the 5264-update_chain_codec branch from d1989ea to d1e6664 Compare May 29, 2020 08:25
@shuse2 shuse2 force-pushed the 5264-update_chain_codec branch from fc03bef to c9ddd65 Compare May 29, 2020 09:24
@shuse2 shuse2 requested review from ManuGowda and ishantiw May 29, 2020 14:13
@shuse2 shuse2 marked this pull request as ready for review May 29, 2020 14:14
@shuse2 shuse2 force-pushed the 5264-update_chain_codec branch from 4f9a4bd to fe4c4dc Compare May 29, 2020 14:15
Copy link
Contributor

@ManuGowda ManuGowda left a comment

Choose a reason for hiding this comment

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

LGTM, few comments

}

/** End: BlockHeaders */

/** Begin: Blocks */

public async getBlockByID(id: string): Promise<BlockInstance> {
const blockJSON = await this._storage.getBlockByID(id);
public async getBlockByID<T>(id: Buffer): Promise<Block<T>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

whats the purpose of generics here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It can specify what type of block header it's expecting

Copy link
Contributor

Choose a reason for hiding this comment

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

but we you care not using T anywhere.. its basically expecting same type it requested for.. so it wouldnt make difference if we remove it as well rite? you are missing this part
return this._decodeRawBlock<T>(block);?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is mainly used outside, maybe we can remove it, but if we want to make typescript work bit better outside of chain, i think this is better?
From outside, i dont think return this._decodeRawBlock<T>(block); matters for the type

elements/lisk-chain/src/schema.ts Outdated Show resolved Hide resolved
elements/lisk-chain/test/utils/transaction.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@ishantiw ishantiw left a comment

Choose a reason for hiding this comment

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

Apart from one comment LGTM

Comment on lines +37 to 41
export interface DefaultAsset {
[key: string]: {
[key: string]: BasicTypes;
};
}
Copy link
Contributor

@ishantiw ishantiw Jun 2, 2020

Choose a reason for hiding this comment

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

can't we define AccountAsset in a more granular way AccountAsset { delegate: {...}, sentVotes: {...}, unlocking: {...}} instead of just giving it so generic. I know its corresponding to the schema provided in the LIP but once we deserialize and make an Account class object maybe we can expand the type definition withing the asset?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is intentionally made generic because chain doesn't know about exact account asset schema

@shuse2 shuse2 merged commit 5b234e0 into feature/update_to_use_codec Jun 2, 2020
@shuse2 shuse2 deleted the 5264-update_chain_codec branch June 2, 2020 10:26
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 lisk-chain to use encoded data to store
3 participants