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

Ownership transaction predicate #290

Merged

Conversation

Projects
None yet
2 participants
@ben-chain
Copy link
Contributor

commented Jun 20, 2019

Description

  • Includes a feature-complete version of the deosit contract conforming to the spec.
  • Implements a TransactionPredicate contract which allows deprecation logic based on a verifyTransaction(preState, transaction, witness, postState). Inherits this contract to build an OwnershipTransactionPredicate implementing verification in accordance with the spec.

Questions

  • Are we okay with the manual decoding of parameters as is done in OwnershipTransactionPredicate.verifyTransaction? Specifically, this is done without structs because I found some jankiness around abi.decode(encoding, struct). I think it's fine for now.

Metadata

Fixes

Blockers

  • Still need to implement sig checking.
  • Still need to explicitly test the originBlock, maxBlock, and newState == postState.stateObject, I have tested these as I was building but need to break them out explicitly into a test for the predicate itself, as all tests right now live in the deposit contract test.

Don't think either is a blocker to merging at this time.

Contributing Agreement

karlfloersch and others added some commits May 25, 2019

Add and test finalizeExit
Includes:
- Removal of depositedRanges after one exits
Merge pull request #1 from ben-chain/master
implement kelvin's review comments

@ben-chain ben-chain requested a review from karlfloersch Jun 20, 2019

ben-chain added some commits Jun 21, 2019

Merge branch 'feat/ownership-transaction-predicate' of github.com:ben…
…-chain/pigi into feat/ownership-transaction-predicate

@karlfloersch karlfloersch referenced this pull request Jun 21, 2019

Closed

Fix state manager `inBlock` usage #293

1 of 1 task complete
@karlfloersch
Copy link
Contributor

left a comment

Mostly looks good! There's a few small changes, but the structure looks reasonable for a first pass.

Note that the tests are failing & I believe it can be fixed by increasing the timeout for the failing tests (change to something like .timeout(1000) or something.

return {
// newState: this.newState.jsonified,
originBlock: hexStringify(this.originBlock),
// maxBlock: hexStringify(this.maxBlock)

This comment has been minimized.

Copy link
@karlfloersch

karlfloersch Jun 21, 2019

Contributor

Remove unused comments

This comment has been minimized.

Copy link
@ben-chain

ben-chain Jun 21, 2019

Author Contributor

fixing--these weren't supposed to be commented 😂

@@ -182,7 +182,7 @@ export const bnToHexString = (bn: BigNum): string => {
export const hexStringify = (value: BigNum | Buffer): string => {
if (value instanceof BigNum) {
return bnToHexString(value)
} else {
} else if (value instanceof Buffer) {
return bufToHexString(value)
}

This comment has been minimized.

Copy link
@karlfloersch

karlfloersch Jun 21, 2019

Contributor

Looks like this may squash errors if you do not give it a BigNum or a Buffer -- best to add throw new Erorr('Unrecognized type') if this else if does not pass

* Represents a basic abi encodable AbiOwnershipParameters
*/
export class AbiOwnershipParameters implements AbiEncodable {
// implements OwnershipParameters, AbiEncodable {

This comment has been minimized.

Copy link
@karlfloersch

karlfloersch Jun 21, 2019

Contributor

Unused comment -- should be removed completely, and if desired a comment (TODO?) which describes why we might want to implement OwnershipParameters

}

/**
* Creates a AbiOwnershipParameters from an encoded AbiOwnershipParameters.

This comment has been minimized.

Copy link
@karlfloersch

karlfloersch Jun 21, 2019

Contributor

Incorrect comment -- should be OwnershipTransaction

const range = AbiRange.from(decoded[3])
return new AbiOwnershipTransaction(
depositAddress,
methodId,

This comment has been minimized.

Copy link
@karlfloersch

karlfloersch Jun 21, 2019

Contributor

Are we keeping methodId? I'm down to do it but clearly it's not required for ownership tx. I'd lean towards removing and then adding it in when we need it for more complex predicates

This comment has been minimized.

Copy link
@karlfloersch

karlfloersch Jun 21, 2019

Contributor

I believe we will also need to change this for the state manager but that can be done in another PR.

methodId: string
parameters: any
range: Range
}

export interface OwnershipParameters {
newState: StateObject

This comment has been minimized.

Copy link
@karlfloersch

karlfloersch Jun 21, 2019

Contributor

I generally like newStateObject for this name as it is more explicit

This comment has been minimized.

Copy link
@ben-chain

ben-chain Jun 21, 2019

Author Contributor

Ok I will make this change, cc @akhila-raju since we just went over as newState


struct Transaction {
address depositAddress;
bytes32 methodId;

This comment has been minimized.

Copy link
@karlfloersch

karlfloersch Jun 21, 2019

Contributor

Do we remove methodId?

struct Transaction {
address depositAddress;
bytes32 methodId;
bytes parameters;

This comment has been minimized.

Copy link
@karlfloersch

karlfloersch Jun 21, 2019

Contributor

Another name for this could be body -- eg. transaction.body

That said parameters is fine as well, just slightly confuses my intuitions if we were to add transaction.parameters.methodId vs transaction.body.methodId ... Not a big deal either way though.

@karlfloersch
Copy link
Contributor

left a comment

Looks like all are addressed!

We will fix the "parameters" / "methodId" later

@karlfloersch karlfloersch merged commit e8df674 into plasma-group:master Jun 21, 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.