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 dependency on blockchain-k-plugin so KEVM can run with Mantis #149

Merged
merged 5 commits into from
Mar 26, 2018

Conversation

dwightguth
Copy link
Member

@dwightguth dwightguth commented Mar 5, 2018

This PR should make all the Mantis ethereum-tests tests pass.

Fixes #146

@dwightguth dwightguth added this to To do in Activity via automation Mar 5, 2018
@dwightguth dwightguth moved this from To do to In progress in Activity Mar 5, 2018
@dwightguth dwightguth force-pushed the node2 branch 2 times, most recently from 4a8b068 to 4e9a689 Compare March 5, 2018 17:30
@dwightguth dwightguth requested a review from ehildenb March 5, 2018 18:09
@ehildenb
Copy link
Member

@dwightguth initial review:

  • Does the submodule need to be at /plugin? Is it ok to hide it in .build/plugin? I would prefer it.
  • Please put && on the next line in the Makefile when doing split lines, indented from the previous line (as the style was before the change). This makes it easy to delete a step by deleting a single line.

I will need more time to read the K code in detail.

@dwightguth
Copy link
Member Author

@ehildenb I fixed the issue with the lines in the Makefile, but I think that the submodule should be left where it is, because unlike the submodules under .build, the plugin isn't really a dependency, it's actually part of the code of the project, but it lives in a submodule simply because that code is shared with another project.

Copy link
Member

@ehildenb ehildenb left a comment

Choose a reason for hiding this comment

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

Needs more documentation. I think we need to meet in person and discuss the overall architecture to make sure I understand everything.

evm-node.md Outdated
requires N <Int 0 orBool N >=Int 256

syntax EthereumSimulation ::= runVM(iscreate: Bool, to: Int, from: Int, code: String, args: String, value: Int, gasprice: Int, gas: Int, beneficiary: Int, difficulty: Int, number: Int, gaslimit: Int, timestamp: Int, unused: String)

Copy link
Member

Choose a reason for hiding this comment

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

I've never seen this syntax for K. Is it some sort of record syntax?

Just it just enable the line below (matching on a subset of arguments?)

Copy link
Member Author

Choose a reason for hiding this comment

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

yes

@@ -0,0 +1,149 @@
```{.k .node}
Copy link
Member

Choose a reason for hiding this comment

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

This file needs to be documented properly (or at least at all).

evm-node.md Outdated

syntax String ::= contractBytes(WordStack) [function]
// -----------------------------------------------------
rule contractBytes(WS) => #unparseByteStack(WS)
Copy link
Member

Choose a reason for hiding this comment

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

contractBytes == #unparseByteStack? Is the indirection for some compatibility layer?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes

evm-node.md Outdated
syntax Int ::= #getBalance(Int) [function, hook(MANTIS.getBalance)]
| #getNonce(Int) [function, hook(MANTIS.getNonce)]
syntax Bool ::= #isCodeEmpty(Int) [function, hook(MANTIS.isCodeEmpty)]
| #accountExists(Int) [function, hook(MANTIS.accountExists)]
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a huge fan of the hooks being MANTIS.*, because they shouldn't be Mantis specific, should they?

Makefile Outdated
DLLEXT=cmxs
OCAMLC=opt -O3
LIBFLAG=-shared
endif
Copy link
Member

Choose a reason for hiding this comment

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

Can this be indented (perhaps with spaces if not tabs)? Looks strange.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@dwightguth
Copy link
Member Author

Jenkins: test this please

@dwightguth
Copy link
Member Author

Jenkins: test this please

1 similar comment
@dwightguth
Copy link
Member Author

Jenkins: test this please

@dwightguth
Copy link
Member Author

Jenkins: test this please

@dwightguth
Copy link
Member Author

@ehildenb please re-review.

@ehildenb ehildenb merged commit 191fae7 into master Mar 26, 2018
Activity automation moved this from In progress to Done Mar 26, 2018
@ehildenb ehildenb deleted the node2 branch March 26, 2018 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Activity
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants