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

235 - Implementing StateManager.executeTransaction(...) #276

Conversation

Projects
None yet
4 participants
@willmeister
Copy link
Collaborator

commented Jun 12, 2019


Fixes #235

Description

  • Adds StateManager and all of the data types it depends on
  • Adds various dependencies (PluginManager, PredicatePlugin, StateDB, etc.)
  • Implements the StateManager.executeTansaction(...) method, leaning on mocked dependency interfaces for now

willmeister added some commits Jun 11, 2019

* Moving data-types defined for state manager back within the scope o…
…f the core package and out of utils.

* Moving range-store data types into range-store interface to keep interface-level data-type access from referencing implementation code
* Adding default StateManager with dummy implementation
- Adding StateDB and types it depends on
- Injecting StateDB into StateManager
* Adding PluginManager and PredicatePlugin interfaces to satisfy Stat…
…eManager dependencies

* Injecting PredicateManager into StateManager
* First pass at StateManager.executeTransaction(...)

Still need to write unit tests

willmeister added some commits Jun 12, 2019

* Moving data-types defined for state manager back within the scope o…
…f the core package and out of utils.

* Moving range-store data types into range-store interface to keep interface-level data-type access from referencing implementation code
* Adding default StateManager with dummy implementation
- Adding StateDB and types it depends on
- Injecting StateDB into StateManager
* Adding PluginManager and PredicatePlugin interfaces to satisfy Stat…
…eManager dependencies

* Injecting PredicateManager into StateManager
* First pass at StateManager.executeTransaction(...)

Still need to write unit tests
Merge branch 'feat/235/StateManager-ImplementExecuteTransaction' of g…
…ithub.com:willmeister/pigi into feat/235/StateManager-ImplementExecuteTransaction

@kfichter kfichter added the @pigi/core label Jun 12, 2019

@kfichter kfichter self-requested a review Jun 12, 2019

@kfichter kfichter added the WIP label Jun 12, 2019

@kfichter
Copy link
Contributor

left a comment

Nice!!! Looking dope. Added some comments 👍.

Show resolved Hide resolved packages/core/src/app/client/index.ts Outdated
Show resolved Hide resolved packages/core/src/app/client/state-db.ts Outdated
Show resolved Hide resolved packages/core/src/app/client/state-manager.ts Outdated
Show resolved Hide resolved packages/core/src/app/client/state-manager.ts Outdated
* @param address the address of the PredicatePlugin
* @returns the PredicatePlugin if one is associated with the provided Address, else undefined
*/
getPlugin(address: string): Promise<PredicatePlugin | undefined>

This comment has been minimized.

Copy link
@kfichter

kfichter Jun 12, 2019

Contributor

General question for other reviewers/author: does anyone know if it's considered better practice to return undefined or to throw an error when the specific plugin doesn't exist?

This comment has been minimized.

Copy link
@willmeister

willmeister Jun 12, 2019

Author Collaborator

Same question for error cases in StateManager.executeTransaction(...) as well as in general.

I suppose they all require checks after method invocation, but neither convey information very well unless we start defining our own Errors or wrap our return objects in a Result<T> that contains custom error code and message or something.

cc: @karlfloersch @ben-chain

This comment has been minimized.

Copy link
@karlfloersch

karlfloersch Jun 13, 2019

Contributor

Throwing an error is a bit more expressive & is done in Level so it's probably a bit better.

I suppose they all require checks after method invocation.... start defining our own Errors

Yeah agreed. My guess is defining a custom error is probably the "best practice"

stateUpdate: StateUpdate
}

// TODO: Define this properly if not `string`. Just adding it to be able to define StateQuery.

This comment has been minimized.

Copy link
@kfichter

kfichter Jun 12, 2019

Contributor

Proper definition is added in this PR, so we can change this as soon as the PR is merged.

Show resolved Hide resolved packages/core/src/app/client/state-manager.ts Outdated
Show resolved Hide resolved packages/core/src/app/client/state-manager.ts

willmeister added some commits Jun 12, 2019

* Adding linting to core package
* Fixing core package linting failures
* Pulling type guards out of StateManager and into type-guards
* Added another unit test to StateManager Tests

@willmeister willmeister changed the title [ WIP don't merge yet :) ] Implementing StateManager.executeTransaction(...) 235 - Implementing StateManager.executeTransaction(...) Jun 13, 2019

@ben-chain

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

Nice, this is coming along!! :D I think we need to change the Transaction interface though--I have been using something different here.

We at least need these parameters for the ownership transaction. Technically, we also have a methodId in the spec right now, but I'm curious if it's not a good idea to just wrap this into a parameters field which encompasses all the custom data for that predicate's transaction type. So for predicates which have multiple transaction methods, we could add a methodId as the first parameter but omit it otherwise. Would be curious of everyones' thoughts on that!

@ben-chain

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

Was just re-reviewing this morning and caught another small thing--I believe lines 65/66 should be removed. The reason is that these transactions do not have to be a sub- or super-set of the verified ranges, we just only update their intersection.

For example, If we have only verified [10, 20) and get a transaction resulting in some SU on [0,30), then we still update [10, 20) as verified into the next block. Or, if we have verified [0, 30) and get a transaction resulting in an SU on [10, 20), we still count [10, 20) as verified into the next block. This is ensured here.

(Which reminds me--is there an easy way to check that start < end any time we instantiate a Range? In this case, because getVerified... is pulling only ranges which intersect the transaction, we don't have to worry about it. But in general this could save some headaches.)

@willmeister

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 13, 2019

Was just re-reviewing this morning and caught another small thing--I believe lines 65/66 should be removed. The reason is that these transactions do not have to be a sub- or super-set of the verified ranges, we just only update their intersection.

For example, If we have only verified [10, 20) and get a transaction resulting in some SU on [0,30), then we still update [10, 20) as verified into the next block. Or, if we have verified [0, 30) and get a transaction resulting in an SU on [10, 20), we still count [10, 20) as verified into the next block. This is ensured here.

(Which reminds me--is there an easy way to check that start < end any time we instantiate a Range? In this case, because getVerified... is pulling only ranges which intersect the transaction, we don't have to worry about it. But in general this could save some headaches.)

@ben-chain the intention here is to make sure that they do intersect, not that they're sub-/super-sets of each other. If range1.end < range2.start or range1.start > range2.end, there is 0 overlap, right?

Range is just a datatype interface, so I'm not sure that's possible at construction time unless we use a factory for it. The alternative is to use isValidRange or something similar when validation is necessary.

@ben-chain

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

Ahhhhhh I see--I misread this, I thought it was comparing a start to a start and end to an end. This is right then, nice!!!

* Adding non-subset test for StateManager
* Passing block number into PredicatePlugin.executeStateTransition(...)
* Removing unnecessary block number checks in StateManager
* Updating data-types to match spec
* Updating StateManager and related tests to populate correct data types
@ben-chain

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

Amazing!!! :D I think this is getting close to a merge. Two things:

  1. We need an arbitrary parameters: bytes in the transactions
  2. I think the way to pass the block number into the predicate plugin's transition function is to pass in an arbitrary block number inBlock which tells executeTransaction which block to execute the transaction in. This would be the thing checked against verifiedBlockNumber + 1 and also passed to the predicate plugin.
@karlfloersch

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

If range1.end < range2.start or range1.start > range2.end

There's also an intersects() function in rangeDB, so it might be good to pull it out and put it in a shared library considering it's such common functionality --

/**
* Checks if two ranges intersect, eg. [1,10) & [8,11) would return true.
* @param start1 The start of the first range.
* @param end1 The end of the first range.
* @param start2 The start of the second range.
* @param end2 The end of the second range.
* @returns true if max(start1, start2) < min(end1, end2), false otherwise.
*/
private intersects(
start1: Buffer,
end1: Buffer,
start2: Buffer,
end2: Buffer
): boolean {
const maxStart = bufferUtils.max(start1, start2)
const minEnd = bufferUtils.min(end1, end2)
return bufferUtils.lt(maxStart, minEnd)
}

@willmeister willmeister removed the WIP label Jun 13, 2019

@willmeister

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 13, 2019

Amazing!!! :D I think this is getting close to a merge. Two things:

  1. We need an arbitrary parameters: bytes in the transactions
  2. I think the way to pass the block number into the predicate plugin's transition function is to pass in an arbitrary block number inBlock which tells executeTransaction which block to execute the transaction in. This would be the thing checked against verifiedBlockNumber + 1 and also passed to the predicate plugin.
  1. That's there, just as a string. I can update it.
  2. Can add this, no problem 👍
@willmeister

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 13, 2019

If range1.end < range2.start or range1.start > range2.end

There's also an intersects() function in rangeDB, so it might be good to pull it out and put it in a shared library considering it's such common functionality --

/**
* Checks if two ranges intersect, eg. [1,10) & [8,11) would return true.
* @param start1 The start of the first range.
* @param end1 The end of the first range.
* @param start2 The start of the second range.
* @param end2 The end of the second range.
* @returns true if max(start1, start2) < min(end1, end2), false otherwise.
*/
private intersects(
start1: Buffer,
end1: Buffer,
start2: Buffer,
end2: Buffer
): boolean {
const maxStart = bufferUtils.max(start1, start2)
const minEnd = bufferUtils.min(end1, end2)
return bufferUtils.lt(maxStart, minEnd)
}

Adding this to #278 since it falls under better sharing / utils.

@kfichter kfichter changed the title 235 - Implementing StateManager.executeTransaction(...) [WIP] 235 - Implementing StateManager.executeTransaction(...) Jun 13, 2019

@kfichter kfichter changed the title [WIP] 235 - Implementing StateManager.executeTransaction(...) 235 - Implementing StateManager.executeTransaction(...) Jun 13, 2019

willmeister added some commits Jun 14, 2019

@ben-chain ben-chain self-requested a review Jun 15, 2019

@ben-chain
Copy link
Contributor

left a comment

Woohoo!!!

This is great--there are just a few small things to change:

  • pass inBlock to the predicate plugin, not verifiedBlockNumber
  • minor naming changes to maintain consistency with contracts

I'm gonna go ahead and merge then make a small PR with those changes :D

@ben-chain ben-chain merged commit 176b94e into plasma-group:master Jun 15, 2019

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.