Conversation
facuspagnuolo
left a comment
There was a problem hiding this comment.
I think we shouldn't apply some of these changes. Ideally, we were thinking to upgrade the test suites to Truffle 5 + web3 1.x along with adopting these test-helpers
| "web3": "^1.0.0" | ||
| }, | ||
| "dependencies": { | ||
| "chai": "^4.2.0", |
There was a problem hiding this comment.
This one is marked as peer dep I think, so it shouldn't be here
|
|
||
| function getNewProxyAddress(receipt) { | ||
| return getEventArgument(receipt, 'NewAppProxy', 'proxy') | ||
| } |
There was a problem hiding this comment.
Isn't getInstalledApp enough? Look at this example
| v: signedPayload.substr(130, 2) == '00' ? 27 : 28, | ||
| }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Part of the idea of the whole migration to test helpers was to do it while upgrading to Truffle 5, carrying web3 1.x with it. Thus, I think we should remove the promisified versions
| } | ||
| if(error.message.includes(THROW_ERROR_PREFIX_V4)) { | ||
| error.reason = error.message.replace(THROW_ERROR_PREFIX_V4, '').trim() | ||
| } |
There was a problem hiding this comment.
See below my comment about Truffle 5
There was a problem hiding this comment.
So, if we are assuming that we are always using truffle 5, shouldn’t remove the if completely? It is supposed to be for truffle 4 (see comment above), but it was not working for me without this fix.
There was a problem hiding this comment.
Yes, good point, let's remove the if and align just on v5!
This reverts commit 1a0f688.
| } | ||
| if(error.message.includes(THROW_ERROR_PREFIX_V4)) { | ||
| error.reason = error.message.replace(THROW_ERROR_PREFIX_V4, '').trim() | ||
| } |
There was a problem hiding this comment.
Yes, good point, let's remove the if and align just on v5!
| function decodeEvents(receipt, contractAbi, eventName) { | ||
| const rawLogs = | ||
| receipt.rawLogs || (receipt.receipt && receipt.receipt.rawLogs) || [] | ||
| receipt.rawLogs || (receipt.receipt && receipt.receipt.rawLogs) || receipt.logs || [] |
There was a problem hiding this comment.
Not sure I understand this; the receipt.logs are already decoded, right?
| async function isGanache(ctx) { | ||
| const isNodeResult = await isNode(ctx, GANACHE_NODE_ID) | ||
| if (isNodeResult === undefined) { | ||
| return web3.currentProvider.host === 'http://localhost:8545' |
There was a problem hiding this comment.
Not sure if this is really safe, as a local geth would also be running on this host and port by default, right?
| injectWeb3, | ||
| ...addresses, | ||
| ...aragonOS, | ||
| ...asserts, |
There was a problem hiding this comment.
Ah, I had actually wanted to avoid re-exporting these directly from index.js, especially not destructured.
I was hoping to get something like:
import { ANY_ENTITY } from '@aragon/contract-helpers-test/aragon-os'
For the user, but this turned out difficult to do.
Would appreciate any other suggestions, but otherwise I wonder if we should keep it for now as:
import { ANY_ENTITY } from '@aragon/contract-helpers-test/aragon-os/src/aragon-os'
Or export the entire object out, rather than destructuring it, so it looks like this to the user:
import { aragonOS } from '@aragon/contract-helpers-test/aragon-os'
const { ANY_ENTITY } = aragonOS
There was a problem hiding this comment.
Is this to optimize? Does it have a great impact in performance?
I did it this way to not have to remember the path of those methods, which is less convenient.
There was a problem hiding this comment.
Not at all, I wanted to provide some structure to group some utilities / asserts as not all of them may be useful in a vanilla project (e.g. Staking).
But yes, thinking about this, perhaps exporting the base assert part of the library would be better, and the only level of separation we have is with aragon-os (and perhaps in the future, aragon-court)!
| // Truffle v5 provides `error.reason`, but v4 does not. | ||
| if (!error.reason && error.message.includes(THROW_ERROR_PREFIX)) { | ||
| error.reason = error.message.replace(THROW_ERROR_PREFIX, '').trim() | ||
| if (!error.reason) { |
There was a problem hiding this comment.
Doing the migration to use buidler I notice that when running tests with the buidler-evm the error is not an object and just a string like the following:
Error: VM Exception while processing transaction: revert INIT_ALREADY_INITIALIZED
Followed by the error stack trace, for example:
at <UnrecognizedContract>.<unknown> (0xc9495a8dab51533490bf09007daf60c7a1fc207d)
at <UnrecognizedContract>.<unknown> (0xc9495a8dab51533490bf09007daf60c7a1fc207d)
at TruffleContract.initialize (/Users/gabi/code/Aragon/aragon-apps/node_modules/@nomiclabs/truffle-contract/lib/execute.js:169:26)
at Context.<anonymous> (/Users/gabi/code/Aragon/aragon-apps/apps/finance/test/finance.js:128:32)
at callFn (/Users/gabi/code/Aragon/aragon-apps/node_modules/mocha/lib/runnable.js:374:21)
at Test.Runnable.run (/Users/gabi/code/Aragon/aragon-apps/node_modules/mocha/lib/runnable.js:361:7)
at Runner.runTest (/Users/gabi/code/Aragon/aragon-apps/node_modules/mocha/lib/runner.js:619:10)
at /Users/gabi/code/Aragon/aragon-apps/node_modules/mocha/lib/runner.js:745:12
at next (/Users/gabi/code/Aragon/aragon-apps/node_modules/mocha/lib/runner.js:536:14)
at /Users/gabi/code/Aragon/aragon-apps/node_modules/mocha/lib/runner.js:546:7
at next (/Users/gabi/code/Aragon/aragon-apps/node_modules/mocha/lib/runner.js:448:14)
at /Users/gabi/code/Aragon/aragon-apps/node_modules/mocha/lib/runner.js:509:7
at done (/Users/gabi/code/Aragon/aragon-apps/node_modules/mocha/lib/runnable.js:314:5)
at /Users/gabi/code/Aragon/aragon-apps/node_modules/mocha/lib/runnable.js:379:11
at runMicrotasks (<anonymous>)
at processTicksAndRejections (internal/process/task_queues.js:97:5)
As a consequence every test that is currently using our assertRevert fails. Wonder if we may want to consider this case?
There was a problem hiding this comment.
Yes, let's add a case for it.
|
Closing in favor of aragon/aragonOS@c5cc374 |
No description provided.