-
Notifications
You must be signed in to change notification settings - Fork 366
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 utility script for generating transaction data field #183
Conversation
buildTransactionData.js
Outdated
|
||
const abi = require('ethereumjs-abi') | ||
|
||
function encodeCall(name, arguments, values) { |
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 we share this code with where it is being used in the tests - just don't want to repeat it in more than one place.
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 problem with requiring it from TokenTestUtils is that file uses artifacts.require so it has to be run within truffle tests, whereas this is a standalone script that can be run by node. We could factor out encodeCall to somewhere else and have both this file and TokenTestUtils import from there. It will just involve changing the import on all the tests that use encodeCall
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.
OK. In this case then we should define encodeCall in a separate file as you say and have both import the file.
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.
If we made this a truffle script, could we use the artifacts so we wouldn't need the type parameters anymore?
var FiatTokenV2 = artifacts.require('FiatTokenV2');
FiatTokenV2[name].request(param1, param2, ...)
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.
Confirmed as done.
buildTransactionData.js
Outdated
@@ -0,0 +1,44 @@ | |||
// Description: |
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 we put this in a scripts
directory - somewhere other than the root dir?
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.
Confirmed as done.
scripts/buildTransactionData.js
Outdated
|
||
module.exports = async function(callback) { | ||
await run(); | ||
} |
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, but the truffle docs say we should call the callback
at the end of the script
https://truffleframework.com/docs/getting_started/scripts
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.
confirmed as done.
args = args.slice(1) | ||
|
||
var data = tokenVX[func].request.apply(null, args)["params"][0]["data"] | ||
console.log(data) |
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.
Nice - so we could use this with any contracts we have. That will be super useful.
test/MiscTests.js
Outdated
var slot8Data = await web3.eth.getStorageAt(newProxy.address, 8); | ||
assert.equal("0x00", slot8Data); | ||
}); | ||
|
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.
What are these other changes here and below? It looks like commits that have already been merged into master? Could you clean up the commit history so we just have your changes in the history and the diff?
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.
Confirmed as done.
…for upgradeToAndCall
All requested changes confirmed as completed.
No description provided.