Skip to content
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(test-helpers): remove test-helpers part one: contracts folder #1977

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion apps/address-book/app/app-state-reducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,5 @@ const appStateReducer = state => {
...state
}
}

export default appStateReducer
8 changes: 4 additions & 4 deletions apps/address-book/public/meta/audit.md
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,12 @@ Stages of the audit were as follows:
### CRITICAL

Not found

### MAJOR
Not found

### WARNINGS

Not found

### COMMENTS
Expand All @@ -72,7 +72,7 @@ We recommend adding the explicit check `isInitialized`.

2\. [AddressBook.sol#L91](https://github.com/AutarkLabs/planning-suite/blob/66a851551888ba7eadaab6a5f037048655bc5d88/apps/address-book/contracts/AddressBook.sol#L91)

There is a constant for this kind of error message - `ERROR_CID_MALFORMED`. We recommend factoring out the entire check as a modifier.
There is a constant for this kind of error message - `ERROR_CID_MALFORMED`. We recommend factoring out the entire check as a modifier.

*Fixed at [ed2f199](https://github.com/AutarkLabs/planning-suite/blob/ed2f199ddda280d1e7033648b69399547f05eec7/apps/address-book/contracts/AddressBook.sol)*

Expand All @@ -95,7 +95,7 @@ The important thing to understand is that the memory layout of the next version

It is expected that structured content objects for the entries will be stored in IPFS. Users and developers should keep in mind that IPFS does not guarantee data availability. After some time unused data is removed from IPFS unless explicitly pinned by some node.

*Acknowledged, dev notes were created at [AddressBook.sol#L60](https://github.com/AutarkLabs/planning-suite/blob/4f3f11c194b285f8e407e3cd4def56afb418b233/apps/address-book/contracts/AddressBook.sol#L60)*
*Acknowledged, dev notes were created at [AddressBook.sol#L60](https://github.com/AutarkLabs/planning-suite/blob/4f3f11c194b285f8e407e3cd4def56afb418b233/apps/address-book/contracts/AddressBook.sol#L60)*


6\. [AddressBook.sol#L64](https://github.com/AutarkLabs/planning-suite/blob/66a851551888ba7eadaab6a5f037048655bc5d88/apps/address-book/contracts/AddressBook.sol#L64)
Expand Down
2 changes: 1 addition & 1 deletion apps/allocations/app/store/allocation.js
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ const getRecipientData = (_accountId, _payoutId) => {
return app.call('getNumberOfCandidates', _accountId, _payoutId)
.pipe(
first(),
mergeMap(candidateLength =>
mergeMap(candidateLength =>
range(0, candidateLength)
),
mergeMap(candidateIndex => (
Expand Down
12 changes: 6 additions & 6 deletions apps/allocations/public/meta/audit.md
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ Not found

### MAJOR
Not found

### WARNINGS

1\. [Allocations.sol#L463](https://github.com/AutarkLabs/planning-suite/blob/5be80e35f6e8e2c58f2b1b0f95f43baf40886507/apps/allocations/contracts/Allocations.sol#L463)
Expand Down Expand Up @@ -96,7 +96,7 @@ The cycle will be aborted and the transaction will be rolled back if there are `

5\. [Allocations.sol#L358](https://github.com/AutarkLabs/planning-suite/blob/5be80e35f6e8e2c58f2b1b0f95f43baf40886507/apps/allocations/contracts/Allocations.sol#L358)

New periods are not being initialized. We suggest adding the `transitionsPeriod` modifier.
New periods are not being initialized. We suggest adding the `transitionsPeriod` modifier.

*Fixed at
[26e6d37](https://github.com/AutarkLabs/open-enterprise/commit/26e6d3766393ed2d12fc57471b56d42c4a680fef)*
Expand All @@ -121,7 +121,7 @@ When calling `runPayout`, `paid` should be passed externally, otherwise this var

### COMMENTS

1\. [Allocations.sol#L40-L45](https://github.com/AutarkLabs/planning-suite/blob/5be80e35f6e8e2c58f2b1b0f95f43baf40886507/apps/allocations/contracts/Allocations.sol#L40-L45)
1\. [Allocations.sol#L40-L45](https://github.com/AutarkLabs/planning-suite/blob/5be80e35f6e8e2c58f2b1b0f95f43baf40886507/apps/allocations/contracts/Allocations.sol#L40-L45)

Constants can be calculated in advance.

Expand Down Expand Up @@ -149,7 +149,7 @@ Gas consumption required to save `Account` can be reduced. Place `uint64 payouts

There’s no need to use the `uint64` key instead of the `uint` one as element adding requires the same amount of gas as `uint`.

*Acknowledged*
*Acknowledged*

5\. [Allocations.sol#L95](https://github.com/AutarkLabs/planning-suite/blob/5be80e35f6e8e2c58f2b1b0f95f43baf40886507/apps/allocations/contracts/Allocations.sol#L95)

Expand Down Expand Up @@ -314,7 +314,7 @@ This check should also involve `_recurrences`, as at the moment it does not refl
[26e6d37](https://github.com/AutarkLabs/open-enterprise/commit/26e6d3766393ed2d12fc57471b56d42c4a680fef)*


23\. [Allocations.sol#L76](https://github.com/AutarkLabs/planning-suite/blob/5be80e35f6e8e2c58f2b1b0f95f43baf40886507/apps/allocations/contracts/Allocations.sol#L76)
23\. [Allocations.sol#L76](https://github.com/AutarkLabs/planning-suite/blob/5be80e35f6e8e2c58f2b1b0f95f43baf40886507/apps/allocations/contracts/Allocations.sol#L76)

If it is not planned to set the budget for the account equal to 0, then `hasBudget` can be omitted, as the `budget != 0` comparison will be equivalent to `hasBudget`.

Expand Down Expand Up @@ -368,7 +368,7 @@ We recommend adding extra checks:

Zero initialization is unnecessary.

*Fixed at
*Fixed at
[26e6d37](https://github.com/AutarkLabs/open-enterprise/commit/26e6d3766393ed2d12fc57471b56d42c4a680fef)*


Expand Down
2 changes: 1 addition & 1 deletion apps/allocations/public/meta/details.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
The Allocations app is used to manage multi-recipient financial allocations that are budget-controlled. Budgets represent spending limits on categorized allocation payments.
The Allocations app is used to manage multi-recipient financial allocations that are budget-controlled. Budgets represent spending limits on categorized allocation payments.

## Features
- Create and edit budgets that set spending limits on allocation categories.
Expand Down
4 changes: 2 additions & 2 deletions apps/allocations/test/Allocations.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,7 @@ contract('Allocations', accounts => {
(web3.toWei(0.01, 'ether') * SUPPORTS[2]) / TOTAL_SUPPORT,
'bossk expense'
)

await app.runPayout(accountId, ethPayoutId)

// calling runPayout has no effect
Expand Down Expand Up @@ -415,7 +415,7 @@ contract('Allocations', accounts => {

it('execute single payout by candidate', async () => {
let empireBalance = await web3.eth.getBalance(empire)

//Repay gas costs to empire
const costs = (
empireInitialBalance.toNumber()
Expand Down
2 changes: 1 addition & 1 deletion apps/dot-voting/app/utils/useUserVoteStats.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import tokenBalanceOfAbi from '../abi/token-balanceof.json'
const tokenAbi = [].concat(tokenBalanceOfAbi)

// TODO: apply cleanups to the useEffects since this is generating errors in the browser:
/*
/*
react-dom.development.js:558 Warning: Can't perform a React state update on an unmounted
component. This is a no-op, but it indicates a memory leak in your application. To fix,
cancel all subscriptions and asynchronous tasks in a useEffect cleanup function.
Expand Down
2 changes: 1 addition & 1 deletion apps/dot-voting/contracts/DotVoting.sol
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import "@aragon/apps-shared-minime/contracts/MiniMeToken.sol";

// TODO: Revert import path when changes get merged into aragon/os
// import "@aragon/os/contracts/common/ADynamicForwarder.sol";
import "@tps/test-helpers/contracts/common/ADynamicForwarder.sol";
import "./lib/ADynamicForwarder.sol";


contract DotVoting is ADynamicForwarder, AragonApp {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,21 @@ import "@aragon/os/contracts/lib/math/SafeMath.sol";
import "@aragon/os/contracts/lib/math/SafeMath64.sol";

// TODO: Use @aragon/os/contracts/ version when it gets merged
import "../evmscript/DynamicScriptHelpers.sol";
import "./DynamicScriptHelpers.sol";
// TODO: Research why using the @aragon/os version breaks coverage
import "@aragon/os/contracts/common/IForwarder.sol";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not re-implement this until we decide how to proceed with renaming this import during coverage

// import "@aragon/os/contracts/common/IForwarder.sol";

interface InterfacedForwarder {
function isForwarder() external pure returns (bool);

// TODO: this should be external
// See https://github.com/ethereum/solidity/issues/4832
function canForward(address sender, bytes evmCallScript) public view returns (bool);

// TODO: this should be external
// See https://github.com/ethereum/solidity/issues/4832
function forward(bytes evmCallScript) public;
}

/**
* @title ADynamicForwarder App
Expand All @@ -21,7 +33,7 @@ import "@aragon/os/contracts/common/IForwarder.sol";
*/


contract ADynamicForwarder is IForwarder {
contract ADynamicForwarder is InterfacedForwarder {
using DynamicScriptHelpers for bytes;
using SafeMath for uint256;
using SafeMath64 for uint64;
Expand Down
2 changes: 0 additions & 2 deletions apps/dot-voting/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,7 @@
"frontend": "npm run sync-assets && parcel app/index.html --port 4444",
"ganache-cli:test": "sh ../../node_modules/@aragon/test-helpers/ganache-cli.sh",
"lint": "solium --dir ./contracts",
"postcoverage": " sed -i 's+./IForwarder.sol+@aragon/os/contracts/common/IForwarder.sol+g' ../shared/test-helpers/contracts/common/ADynamicForwarder.sol",
"precommit": "lint-staged",
"precoverage": "sed -i 's+@aragon/os/contracts/common/IForwarder.sol+./IForwarder.sol+g' ../shared/test-helpers/contracts/common/ADynamicForwarder.sol",
Copy link
Collaborator

@topocount topocount Feb 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't make sense to remove this since it addresses a bug in the coverage tool. We should upgrade our coverage tool before removing this since it doesn't make sense to create our own duplicated import when it already exists in a dependency we're using

"prepublishOnly": "truffle compile",
"publish:cd": "../../shared/deployments/check-publish.sh",
"publish:http": "npm run build:script && yes | aragon apm publish major --files dist --http localhost:4444 --http-served-from ./dist --propagate-content false --skip-confirmation true",
Expand Down
Loading