-
-
Notifications
You must be signed in to change notification settings - Fork 17
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
implement Hofmann structure #169
Conversation
why you are removing fixtures? 🤔 |
This is very much WIP. I made the PR so I have a nice diff to look at. I will announce when it's ready for review. |
cool. go go go 🚀 |
i have this one failing test:
seems easy to fix, need help? |
Still WIP @johannbarbie. patience :) |
Ok all tests now passing and it's cleaned up a bit. FIrst comments welcome! |
i still have these guys failing. errors look like this:
|
Yeah I noticed this after, seems like I need to update the off-chain stepper to behave the same. |
is that even possible? or are we at a dead end here? |
Why not possible? I just didn't even know it existed in this repo. Will do it tomorrow. |
|
||
|
||
contract EVMRuntime is EVMConstants { | ||
using EVMMemory for EVMMemory.Memory; | ||
using EVMStack for EVMStack.Stack; | ||
using EVMCode for EVMCode.Code; | ||
using EVMTokenBag for EVMTokenBag.TokenBag; | ||
|
||
// bridge has to defreagment the tokens |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor detail, but it is not actually the Bridge
, but the Enforcer
that will have to do these things. see here: https://docs.google.com/drawings/d/1WXcAvTEpXDGeL68MfWOP20mERht8rzqfSzdiFtYZeSc
in that diagram the bridge takes the client contract role.
same in this diagram: https://docs.google.com/drawings/d/1vlRmrHJd-eozM9e34pgCEnm8Iuq9bhqVnopOva7Tfz4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well the bridge should do that and enforcer just passes onwards i guess? Details of plasma should stay out of the EVM-enforcer as much as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, i think we need some intermediate contract here. the bridge only understand transactions, but not runtime.
331690, | ||
765537, | ||
47813, | ||
90953, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how does this one get created?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This entire file is generated during tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we exclude it from git then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe 🤔
I did not take the time to fully understand what it does 😆 , but I assume it is committed for a reason.
contracts/EVMRuntime.sol
Outdated
} | ||
state.gas -= gasFee; | ||
|
||
bytes memory cd = state.mem.toBytes(stack.inOffset, stack.inSize); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe use cData
or callDataBytes
here, to keep it readable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome work 💯
few details and comments required
contracts/EVMRuntime.sol
Outdated
bool success; | ||
bytes memory returnData; | ||
|
||
if (funSig == 0x22334455) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be 0xa9059cbb
for transfer(address,uint256)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed and moved to constants.
contracts/EVMRuntime.sol
Outdated
to: dest, | ||
value: amount | ||
}); | ||
success = state.tokenBag.transfer(params); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could the creation of params be done inline, to save a stack element?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was actually not needed anymore. Removed in favour of normal function params.
contracts/EVMRuntime.sol
Outdated
uint amount; | ||
|
||
assembly { | ||
dest := mload(add(cd,24)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
contracts/EVMRuntime.sol
Outdated
} | ||
|
||
if (!success) { | ||
state.stack.push(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
contracts/EVMRuntime.sol
Outdated
@@ -1609,7 +1690,13 @@ contract EVMRuntime is EVMConstants { | |||
} | |||
state.gas -= retEvm.gas; | |||
|
|||
retEvm.data = state.mem.toBytes(inOffset, inSize); | |||
bytes memory cd = state.mem.toBytes(inOffset, inSize); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
return tokenBag; | ||
} | ||
|
||
module.exports = TokenBagHelpers; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
utils/EVMRuntime.js
Outdated
const success = runState.tokenBag.transfer(color, from, to, amount); | ||
|
||
if (success) { | ||
runState.stack.push(new BN(1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add comment here if 1
is success or error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or use constant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FIXED.
utils/EVMRuntime.js
Outdated
const data = this.memLoad(runState, inOffset, inLength); | ||
const funcSig = this.getFuncSig(data); | ||
|
||
if (funcSig === '0x22334455') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update sigHash
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FIxed.
utils/EVMRuntime.js
Outdated
runState.stack.push(new BN(1)); | ||
runState.returnValue = returnValue; | ||
return; | ||
} else if (funcSig === '0x12341234') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update sigHash
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
const data = this.memLoad(runState, inOffset, inLength); | ||
const funcSig = this.getFuncSig(data); | ||
|
||
if (toAddress.gten(0) && toAddress.lten(8)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the address has to be larger than or equal to 0 and less than or equal 8? 🤔
when does this condition match?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, precompiles, got it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥓
resolves #118