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

Feat/422/persisted state machine #439

Merged

Conversation

@willmeister
Copy link
Collaborator

commented Sep 11, 2019

Description

Creates a persisted state machine for the client, backed by a Sparse Merkle Tree.

Metadata

Fixes

Contributing Agreement

willmeister added 7 commits Sep 9, 2019
…ate machine during async read-modify-write flows
@willmeister willmeister referenced this pull request Sep 12, 2019
1 of 1 task complete
Copy link
Contributor

left a comment

This is straight 🔥🔥🔥🔥 -- amazing!

Left a few comments which go through my thinking of a potential change and ultimately thinking it's better not to make it. Left them there so you can see the reasoning but I think great as is.

}

public async getBalances(account: Address): Promise<Balances> {
const key: BigNumber = this.getAddressKey(account)

This comment has been minimized.

Copy link
@ben-chain

ben-chain Sep 12, 2019

Contributor

Nice--would make it possible to add the lower depth tree stuff down the line easily, love it

return balances
}

public async applyTransaction(

This comment has been minimized.

Copy link
@ben-chain

ben-chain Sep 12, 2019

Contributor

Looks like this is the same/renamed code as Karl's mock stuff, so won't review closely. One question would be whether to have a stronger separation between the "transaction application" state machine vs. the "SMT block persistence" state machine--i.e. break them into two files--so that the transaction logic would be in a separate file than the "supporting logic" of the block structure, merkle updates, etc.

This comment has been minimized.

Copy link
@ben-chain

ben-chain Sep 12, 2019

Contributor

Actually, reading on, the setAddressState below makes the separation pretty clean--so maybe not critical. I guess would be the natural thing to do if this code gets too hefty, but seems like it's cleaner than I thought when writing that 😊

@@ -15,26 +17,32 @@ import { UnipigWallet, MockAggregator, UNI_TOKEN_TYPE } from '../src'
const timeout = 20_000

describe('Mock Client/Aggregator Integration', async () => {

This comment has been minimized.

Copy link
@ben-chain

ben-chain Sep 12, 2019

Contributor

Ahhhh-seeing how this reuses the mock integration tests, I now definitely like using the SMT for the mock stuff. Super sweet!!

@willmeister willmeister merged commit 5c7317a into plasma-group:master Sep 12, 2019
2 checks passed
2 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@willmeister willmeister deleted the willmeister:feat/422/PersistedStateMachine branch Sep 12, 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.