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

Rollup Contracts #448

Merged

Conversation

@karlfloersch
Copy link
Contributor

commented Sep 18, 2019

Description

This PR adds sparse merkle trees to the rollup contracts. It builds heavily on #444 but I figured I might as well split it up a bit.

This PR is making sure that we can

  1. Generate SMT roots [COMPLETE]
  2. VerifyAndStore partial proofs [not yet...]

This will be enough for the contracts & so hopefully wont be too difficult.

Questions

  • None yet.

Metadata

Fixes

Turns out these issues were pretty poorly defined so even though this PR isn't huge it should address all of these.

Contributing Agreement

Copy link
Collaborator

left a comment

Looks good! I left a lot of comments, but feel free to ignore any rename requests that won't make things easier between now and Devcon.

There are a few discrepancies between this and the client that we should discuss. Can search for Important to find them.

/*** Blocks ***/
struct Block {
bytes32 rootHash;
uint blockSize;

This comment has been minimized.

Copy link
@willmeister

willmeister Sep 19, 2019

Collaborator

What is blocksize? If number of transitions, should we label it more intuitively?

packages/contracts/contracts/RollupChain.sol Outdated Show resolved Hide resolved
packages/contracts/contracts/RollupChain.sol Outdated Show resolved Hide resolved
/**
* Submits a new block which is then rolled up.
*/
function submitBlock(bytes[] memory _block) public returns(bytes32) {

This comment has been minimized.

Copy link
@willmeister

willmeister Sep 19, 2019

Collaborator

Would change _block to _dataBlocks or _blockLeafState or something that indicates that it's an array of leaf data.

storageSlots[0] = transition.senderSlot;
storageSlots[1] = transition.recipientSlot;
}
if (transitionType == TRANSFER_TYPE) {

This comment has been minimized.

Copy link
@willmeister

willmeister Sep 19, 2019

Collaborator

else if

storageSlots[0] = transition.senderSlot;
storageSlots[1] = transition.recipientSlot;
}
if (transitionType == SWAP_TYPE) {

This comment has been minimized.

Copy link
@willmeister

willmeister Sep 19, 2019

Collaborator

else if

}

function verifyEmptyStorage(dt.Storage memory _storage) internal pure {
require(_storage.pubkey == 0x0000000000000000000000000000000000000000, "Pubkey of storage slot must be zero");

This comment has been minimized.

Copy link
@willmeister

willmeister Sep 19, 2019

Collaborator

Would make a NULL_ADDRESS constant


function verifyEmptyStorage(dt.Storage memory _storage) internal pure {
require(_storage.pubkey == 0x0000000000000000000000000000000000000000, "Pubkey of storage slot must be zero");
require(_storage.balances[0] == 0, "Uni balance must be zero");

This comment has been minimized.

Copy link
@willmeister

willmeister Sep 19, 2019

Collaborator

Would make UNI_INDEX and PIGI_INDEX constants

}

/************
* Decoding *

This comment has been minimized.

Copy link
@willmeister

willmeister Sep 19, 2019

Collaborator

Important tokenType was (and still is) listed as a bool in all our client encoding/decoding, whereas here it's listed as uint.

This comment has been minimized.

Copy link
@karlfloersch

karlfloersch Sep 23, 2019

Author Contributor

Oh so as it turns out, bool encoding is actually identical to uint, so that's not a big problem. That said, I saw this is changed in the new encoding which this uses now so hopefully not an issue anymore.

One note somewhat related to this is that I am currently doing a very dumb manual conversion from balances being an object with keys of '0', & '1', into balances being an array where the first item is uni and the 2nd is pigi. You can see it here: https://github.com/plasma-group/pigi/pull/448/files#diff-364498878b2bb730b65a6a87f059e45aR475

This is pretty annoying unfortunately.

This comment has been minimized.

Copy link
@willmeister

willmeister Sep 24, 2019

Collaborator

Will this require a change on the client? It should be easy to update [de-]serialization so that we sign it the way you want it but also represent it in JS how we want.

This comment has been minimized.

Copy link
@karlfloersch

karlfloersch Sep 24, 2019

Author Contributor

This is a weird thing where the way you have to pass it in to ethers.js is an array. This is because they do the struct serialization on their end and so they need it in an object which is represented the same way that it is serialized. Additionally, you can't have mappings in structs, so there's no way to represent normal objects. This means that we've got to do this somewhat ugly transformation, or on the client side also use an array.

This comment has been minimized.

Copy link
@willmeister

willmeister Sep 24, 2019

Collaborator

We can easily serialize this to an array in abi-encoders.ts and deserialize into the object we want in abi-parsers.ts. If that'll make it easier on you, let's do that. We can represent it however we want in JS as long as 1) We sign the data as the contract expects it and 2) We have a clear way to convert from JS object to abi-encoded to sign/check signatures.

@karlfloersch karlfloersch force-pushed the karlfloersch:feat/contract_merklization branch from daa0551 to 15cd23c Sep 20, 2019
@karlfloersch karlfloersch force-pushed the karlfloersch:feat/contract_merklization branch from e7440e2 to 1368ca1 Sep 23, 2019
@karlfloersch karlfloersch changed the title [WIP] Contract Sparse Merklization Rollup Contracts Sep 23, 2019
@karlfloersch

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2019

yolo

@karlfloersch karlfloersch merged commit 8634ad7 into plasma-group:master Sep 24, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
This was referenced Sep 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.