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

Add Merkle Tree #1

Merged
merged 31 commits into from
Jan 19, 2024
Merged

Add Merkle Tree #1

merged 31 commits into from
Jan 19, 2024

Conversation

Phanco
Copy link
Member

@Phanco Phanco commented Nov 30, 2023

What was the problem?

This PR resolves #2

How was it solved?

Merkle Tree function implemented, and a script to read balances json

How was it tested?

Test cases passed at lisk-contract side using the merkle tree and root built by this script

@Phanco Phanco self-assigned this Nov 30, 2023
@Phanco Phanco marked this pull request as ready for review November 30, 2023 14:29
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
data/example/balances.json Outdated Show resolved Hide resolved
data/example/create-balances.ts Outdated Show resolved Hide resolved
main.ts Outdated Show resolved Hide resolved
main.ts Outdated Show resolved Hide resolved
src/merkle-tree.ts Outdated Show resolved Hide resolved
main.ts Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the purpose of this file?

Copy link
Member Author

Choose a reason for hiding this comment

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

It signs all claims and store as signatures.json. Of course it is for testing/demonstration purpose

Copy link

Choose a reason for hiding this comment

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

Could this file be refactored in a way that we could use it as a library for generating signatures? And then create another example file to use this library?
I think it would be beneficial to have something like this.

Or do we have a plan to create another issue which will deal with it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question, we can work closely with UI team to see how the sign module should be implemented

Copy link
Member

@shuse2 shuse2 left a comment

Choose a reason for hiding this comment

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

Since in this repo, there will be multiple functionality

  • Library
  • CLI
  • Server?

it would be nice to prepare it as monorepo?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated
mandatoryKeys: string[];
optionalKeys: string[];
payload: string;
Copy link

Choose a reason for hiding this comment

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

Is payload used somewhere?
In merkle-tree-result.json file I can't find it.

Copy link
Member Author

Choose a reason for hiding this comment

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

payload is a field to manually compare with existing data, however OZ's lib does not have a function to generate abi-encoded message so now I have removed it

data/example/merkle-tree-result-simple.json Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@Phanco
Copy link
Member Author

Phanco commented Dec 8, 2023

So sorry the README was not updated accordingly, now it has been updated

@Phanco Phanco requested a review from matjazv December 8, 2023 15:45
src/interface.ts Outdated
numberOfSignatures: number;
mandatoryKeys: Array<string>;
optionalKeys: Array<string>;
payload: string;
Copy link

Choose a reason for hiding this comment

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

Probably here could payload also be removed?

README.md Outdated

Due to the constraint of vm.parseJson in Foundry, fields in `merkle-tree-result` such as decimal field will trigger error.

`merkle-tree-simple.json` is a lightweight version of `merkle-tree-result.json`, which also has sufficient params for claiming.
Copy link

Choose a reason for hiding this comment

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

merkle-tree-simple.json -> merkle-tree-result-simple.json?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
data/example/create-balances.ts Outdated Show resolved Hide resolved
data/example/sign.ts Outdated Show resolved Hide resolved
main.ts Outdated Show resolved Hide resolved
src/interface.ts Outdated Show resolved Hide resolved
src/interface.ts Outdated Show resolved Hide resolved
test/main.spec.ts Outdated Show resolved Hide resolved
test/main.spec.ts Outdated Show resolved Hide resolved
test/main.spec.ts Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
data/example/sign.ts Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/buildTree.ts Outdated Show resolved Hide resolved
src/buildTree.ts Outdated Show resolved Hide resolved
test/buildTree.spec.ts Outdated Show resolved Hide resolved
test/buildTreeJSON.spec.ts Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
@@ -0,0 +1,59 @@
import * as fs from 'fs';
Copy link
Member

Choose a reason for hiding this comment

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

let's keep the file name in snake case (both for src and test)

Copy link
Member

Choose a reason for hiding this comment

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

Also, applications maybe doesn't fit with the contents

README.md Outdated Show resolved Hide resolved
jest.config.js Outdated Show resolved Hide resolved
packages/tree-builder/package.json Outdated Show resolved Hide resolved
packages/tree-builder/src/applications/buildTree.ts Outdated Show resolved Hide resolved
packages/tree-builder/src/applications/buildTreeJSON.ts Outdated Show resolved Hide resolved
@Phanco Phanco force-pushed the 115-implement-merkle-tree-builder branch from 98f9349 to 8f257da Compare January 10, 2024 12:49
@Phanco Phanco force-pushed the 115-implement-merkle-tree-builder branch from 90805d3 to ade43be Compare January 10, 2024 13:26
@Phanco Phanco force-pushed the 115-implement-merkle-tree-builder branch from 5cc70b1 to 28d64d5 Compare January 10, 2024 16:13
@Phanco Phanco force-pushed the 115-implement-merkle-tree-builder branch from 1cafbea to 2a0a7fa Compare January 17, 2024 12:22
@Phanco
Copy link
Member Author

Phanco commented Jan 17, 2024

UPDATE: This PR focuses on Merkle Tree building, the method of account reading will be changed in the another PR.

@shuse2 shuse2 merged commit 83a4f23 into main Jan 19, 2024
1 check passed
@shuse2 shuse2 deleted the 115-implement-merkle-tree-builder branch January 19, 2024 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Merkle Tree Builder for LSK Token Claim
4 participants