Some smaller cleanups (mostly regarding calls) #186
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… codes are transmitted outside this)
holgerd77
reviewed
Aug 22, 2017
| @@ -258,7 +258,7 @@ module.exports = { | |||
|
|
|||
| memStore(runState, memOffset, runState.callData, dataOffset, dataLength) | |||
| // sub the COPY fee | |||
| subGas(runState, new BN(Number(fees.copyGas.v) * Math.ceil(dataLength / 32))) | |||
| subGas(runState, new BN(fees.copyGas.v).imuln(Math.ceil(dataLength / 32))) | |||
| @@ -270,14 +270,14 @@ module.exports = { | |||
|
|
|||
| memStore(runState, memOffset, runState.code, codeOffset, length) | |||
| // sub the COPY fee | |||
| subGas(runState, new BN(fees.copyGas.v * Math.ceil(length / 32))) | |||
| subGas(runState, new BN(fees.copyGas.v).imuln(Math.ceil(length / 32))) | |||
| }, | ||
| EXTCODESIZE: function (address, runState, cb) { | ||
| var stateManager = runState.stateManager | ||
| address = utils.setLengthLeft(address, 20) | ||
| stateManager.getContractCode(address, function (err, code) { | ||
| if (err) return cb(err) | ||
| cb(err, utils.intToBuffer(code.length)) | ||
| cb(null, utils.intToBuffer(code.length)) |
| @@ -386,7 +387,7 @@ module.exports = { | |||
| stateManager.putContractStorage(address, key, value, function (err) { | |||
| if (err) return cb(err) | |||
| runState.contract = stateManager.cache.get(address) | |||
| cb() | |||
| cb(null) | |||
| @@ -473,7 +474,7 @@ module.exports = { | |||
| memLength = utils.bufferToInt(memLength) | |||
| const numOfTopics = runState.opCode - 0xa0 | |||
| const mem = memLoad(runState, memOffset, memLength) | |||
| subGas(runState, new BN(numOfTopics * fees.logTopicGas.v + memLength * fees.logDataGas.v)) | |||
| subGas(runState, new BN(fees.logTopicGas.v).imuln(numOfTopics).iadd(new BN(fees.logDataGas.v).imuln(memLength))) | |||
There was a problem hiding this comment.
You can optionally put some brackets around first multiplication like (new BN(fees.logTopicGas.v).imuln(numOfTopics)) for better readability/consistency, otherwise ok.
| makeCall(runState, options, localOpts, done) | ||
| }) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Hmm, code looks correct, not completely understand the changing semantics here, so probably for someone else to review again.
There was a problem hiding this comment.
The account must exists even for precompiles.
| @@ -670,8 +675,8 @@ module.exports = { | |||
| } else { | |||
| stateManager.getContractCode(toAddress, function (err, code, compiled) { | |||
| if (err) return done(err) | |||
| options.compiled = compiled || false | |||
| @@ -921,7 +930,7 @@ function makeCall (runState, callOptions, localOpts, cb) { | |||
| runState.contract.nonce = new BN(runState.contract.nonce).subn(1) | |||
| } | |||
|
|
|||
| cb(err, Buffer.from([results.vm.exception])) | |||
| cb(null, Buffer.from([results.vm.exception])) | |||
| // save result to the stack | ||
| if (result) { | ||
| // NOTE: Ensure that every stack item is padded to 256 bits. | ||
| // This should be done at every opcode in the future. | ||
| result = utils.setLengthLeft(result, 32) | ||
| runState.stack.push(result) | ||
| } | ||
| cb(err) | ||
|
|
||
| cb() |
There was a problem hiding this comment.
Good catch, though that file has all the callbacks without null, need to tackle it at some point. Also some other methods (parseVmResults) there have an issue passing both err and results. Haven't reviewed it yet.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Depends on #188.