-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
refactor execution context #2491
Conversation
@@ -97,7 +97,7 @@ class CompilerMetadata extends Plugin { | |||
var provider = self.fileManager.currentFileProvider() | |||
var path = self.fileManager.currentPath() | |||
if (provider && path) { | |||
executionContext.detectNetwork((err, { id, name } = {}) => { | |||
self.executionContext.detectNetwork((err, { id, name } = {}) => { |
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.
you should be able to use this
here I think
@@ -106,25 +107,25 @@ class DropdownLogic { | |||
|
|||
fromWei (value, doTypeConversion, unit) { | |||
if (doTypeConversion) { | |||
return executionContext.web3().fromWei(typeConversion.toInt(value), unit || 'ether') | |||
return Web3.utils.fromWei(typeConversion.toInt(value), unit || 'ether') |
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.
Why not using ethers.js instead as we're moving out of Web3 to ethers.js ?
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.
I just wanted to get rid of the using executionContext here avoiding breaking changes as maximum as possible. I agree later we can refactor this to either use web3-utils directly or something else like etherjs, it's quite possible this whole method might disapear though so optimizing now could be premature
@@ -205,7 +204,7 @@ function log (self, tx, receipt) { | |||
} | |||
} | |||
|
|||
function renderKnownTransaction (self, data) { | |||
function renderKnownTransaction (self, data, executionContext) { |
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.
Why not using this
or self
for executionContext
as you're doing in the other files ?
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.
keeping the same behaviour for now
@@ -180,13 +180,13 @@ UniversalDAppUI.prototype.getCallButton = function (args) { | |||
cb(txFeeText, priceStatus) | |||
}, | |||
(cb) => { | |||
executionContext.web3().eth.getGasPrice((error, gasPrice) => { | |||
self.executionContext.web3().eth.getGasPrice((error, gasPrice) => { |
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.
Can you not use this
here ?
src/app/ui/universal-dapp-ui.js
Outdated
@@ -264,7 +264,7 @@ UniversalDAppUI.prototype.getCallButton = function (args) { | |||
} | |||
} | |||
if (lookupOnly) { | |||
var decoded = decodeResponseToTreeView(executionContext.isVM() ? txResult.result.execResult.returnValue : ethJSUtil.toBuffer(txResult.result), args.funABI) | |||
var decoded = decodeResponseToTreeView(self.executionContext.isVM() ? txResult.result.execResult.returnValue : ethJSUtil.toBuffer(txResult.result), args.funABI) |
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.
Can you not use this
here instead of self
?
@@ -62,14 +62,14 @@ class CmdInterpreterAPI { | |||
debug (hash, cb) { | |||
var self = this | |||
delete self.d | |||
executionContext.web3().eth.getTransaction(hash, (error, tx) => { | |||
self.executionContext.web3().eth.getTransaction(hash, (error, tx) => { |
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.
Use this
instead
if (error) return cb(error) | ||
var debugSession = new RemixDebug({ | ||
compilationResult: () => { | ||
return self._deps.compilersArtefacts['__last'].getData() | ||
} | ||
}) | ||
debugSession.addProvider('web3', executionContext.web3()) | ||
debugSession.addProvider('web3', self.executionContext.web3()) |
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
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.
we should not merge without fixing plugin problems
@LianaHus it's fixed now |
476dadd
to
187ff98
Compare
187ff98
to
8c30194
Compare
closing since the changes in this PR were included in #2506 |
stops relying on global execution context and instead pass it as parameter