Skip to content
This repository was archived by the owner on Jul 11, 2019. It is now read-only.

tests: add and follow most of the eslint rules #155

Open
come-maiz wants to merge 2 commits into
base: master
Choose a base branch
from
Open

tests: add and follow most of the eslint rules #155

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
91e0213
tests: add and follow solium rules
Jun 1, 2018
6aa2002
tests: add and follow most of the eslint rules
Jun 1, 2018
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
51 changes: 51 additions & 0 deletions .eslintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
{
"extends" : [
"standard",
"plugin:promise/recommended"
],
"plugins": [
"promise"
],
"env": {
"browser" : true,
"node" : true,
"mocha" : true,
"jest" : true
},
"globals" : {
"artifacts": false,
"contract": false,
"assert": false,
"web3": false
},
"rules": {

// Strict mode
"strict": [2, "global"],

// Code style
"indent": [2, 2],
"quotes": [2, "single"],
"semi": ["error", "always"],
"space-before-function-paren": ["error", "always"],
"no-use-before-define": 0,
"eqeqeq": [2, "smart"],
"dot-notation": [2, {"allowKeywords": true, "allowPattern": ""}],
"no-redeclare": [2, {"builtinGlobals": true}],
"no-trailing-spaces": [2, { "skipBlankLines": true }],
"eol-last": 1,
"comma-spacing": [2, {"before": false, "after": true}],
"camelcase": [2, {"properties": "always"}],
"no-mixed-spaces-and-tabs": [2, "smart-tabs"],
"comma-dangle": [1, "always-multiline"],
"no-dupe-args": 2,
"no-dupe-keys": 2,
"no-debugger": 0,
"no-undef": 2,
"object-curly-spacing": [2, "always"],
"max-len": [2, 120, 2],
Copy link
Contributor

@facuspagnuolo facuspagnuolo Jun 1, 2018

Choose a reason for hiding this comment

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

is this rule shadowing the one defined in .soliumrc.json?

Copy link
Contributor

@facuspagnuolo facuspagnuolo Jun 1, 2018

Choose a reason for hiding this comment

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

sorry, this is JS... I'd fix .soliumrc.json to 120

"generator-star-spacing": ["error", "before"],
"promise/avoid-new": 0,
"promise/always-return": 0
}
}
15 changes: 15 additions & 0 deletions .soliumrc.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
{
"extends": "solium:all",
"plugins": ["security"],
"rules": {
"quotes": ["error", "double"],
"no-empty-blocks": "off",
"indentation": ["error", 2],
"max-len": ["error", 79],
Copy link
Contributor

@facuspagnuolo facuspagnuolo Jun 1, 2018

Choose a reason for hiding this comment

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

let's use 120

"no-constant": ["error"],
"security/enforce-explicit-visibility": ["error"],
"security/no-block-members": ["warning"],
"security/no-inline-assembly": ["warning"],
Copy link
Contributor

@facuspagnuolo facuspagnuolo Jun 1, 2018

Choose a reason for hiding this comment

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

thinking about our Proxy contract, should we be warned since we are using inline-assembly to delegate calls?

Copy link
Author

@come-maiz come-maiz Jun 1, 2018

Choose a reason for hiding this comment

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

Yes, in this repo it's less useful. I like it because it forces us to have a good reason to use assembly, and we can use it as a reminder to put this good reason in a comment on the code.
I see this as a: WARNING: black magic ahead. Use with caution.
I would leave it because it's just three or four blocks of code, and hopefully it will not grow much, but no strong opinion here.

"arg-overflow": ["off"]
}
}
25 changes: 19 additions & 6 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,31 @@
language: node_js
node_js:
- '8'

cache:
directories:
- node_modules
env:
-
- SOLIDITY_COVERAGE=true
matrix:

jobs:
# XXX fast_finish doesn't work with stages yet. See
# https://github.com/travis-ci/travis-ci/issues/8425
# --elopio - 20180531
fast_finish: true
allow_failures:
- env: SOLIDITY_COVERAGE=true
script:
- npm test
include:
# Run the unit test suite three times in parallel.
# The first one gets results faster and is the only one required to pass.
# The second one generates the coverage report.
- stage: unit
script: npm test
- stage: unit
script: npm run test
env: SOLIDITY_COVERAGE=true
# solidity and javascript style tests.
- stage: static
script: npm run lint:sol

notifications:
slack:
rooms:
Expand Down
11 changes: 8 additions & 3 deletions contracts/application/AppDirectory.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import "./versioning/ImplementationProvider.sol";
import "./versioning/ImplementationDirectory.sol";
import "openzeppelin-solidity/contracts/ownership/Ownable.sol";


/**
* @title AppDirectory
* @dev Implementation directory with a standard library as a fallback provider.
Expand Down Expand Up @@ -38,10 +39,14 @@ contract AppDirectory is ImplementationDirectory {
* @return Address where the contract is implemented, or 0 if it is not
* found.
*/
function getImplementation(string contractName) public view returns (address) {
function getImplementation(string contractName)
public view returns (address)
{
address implementation = super.getImplementation(contractName);
if(implementation != address(0)) return implementation;
if(stdlib != address(0)) return stdlib.getImplementation(contractName);
if (implementation != address(0))
return implementation;
if (stdlib != address(0))
return stdlib.getImplementation(contractName);
return address(0);
}

Expand Down
61 changes: 47 additions & 14 deletions contracts/application/BaseApp.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import "../upgradeability/AdminUpgradeabilityProxy.sol";
import "../upgradeability/UpgradeabilityProxyFactory.sol";
import "openzeppelin-solidity/contracts/ownership/Ownable.sol";


/**
* @title BaseApp
* @dev Abstract base contract for upgradeable applications.
Expand All @@ -23,18 +24,14 @@ contract BaseApp is Ownable {
factory = _factory;
}

/**
* @dev Abstract function to return the implementation provider.
* @return The implementation provider.
*/
function getProvider() internal view returns (ImplementationProvider);

/**
* @dev Returns the implementation address for a given contract name, provided by the `ImplementationProvider`.
* @param contractName Name of the contract.
* @return Address where the contract is implemented.
*/
function getImplementation(string contractName) public view returns (address) {
function getImplementation(string contractName)
public view returns (address)
{
Copy link
Contributor

@facuspagnuolo facuspagnuolo Jun 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I don't understand why many lines are being fixed this way when we are working with a max-length of 120 Let's use 120

return getProvider().getImplementation(contractName);
}

Expand All @@ -43,7 +40,10 @@ contract BaseApp is Ownable {
* @param contractName Name of the contract.
* @return Address of the new proxy.
*/
function create(string contractName) public returns (AdminUpgradeabilityProxy) {
function create(string contractName)
public
returns (AdminUpgradeabilityProxy)
{
address implementation = getImplementation(contractName);
return factory.createProxy(this, implementation);
}
Expand All @@ -58,17 +58,31 @@ contract BaseApp is Ownable {
* https://solidity.readthedocs.io/en/develop/abi-spec.html#function-selector-and-argument-encoding.
* @return Address of the new proxy.
*/
function createAndCall(string contractName, bytes data) payable public returns (AdminUpgradeabilityProxy) {
function createAndCall(
string contractName,
bytes data
)
payable
public
returns (AdminUpgradeabilityProxy)
{
address implementation = getImplementation(contractName);
return factory.createProxyAndCall.value(msg.value)(this, implementation, data);
return factory.createProxyAndCall.value(
msg.value)(this, implementation, data);
}

/**
* @dev Upgrades a proxy to the newest implementation of a contract.
* @param proxy Proxy to be upgraded.
* @param contractName Name of the contract.
*/
function upgrade(AdminUpgradeabilityProxy proxy, string contractName) public onlyOwner {
function upgrade(
AdminUpgradeabilityProxy proxy,
string contractName
)
public
onlyOwner
{
address implementation = getImplementation(contractName);
proxy.upgradeTo(implementation);
}
Expand All @@ -83,7 +97,15 @@ contract BaseApp is Ownable {
* called, as described in
* https://solidity.readthedocs.io/en/develop/abi-spec.html#function-selector-and-argument-encoding.
*/
function upgradeAndCall(AdminUpgradeabilityProxy proxy, string contractName, bytes data) payable public onlyOwner {
function upgradeAndCall(
AdminUpgradeabilityProxy proxy,
string contractName,
bytes data
)
payable
public
onlyOwner
{
address implementation = getImplementation(contractName);
proxy.upgradeToAndCall.value(msg.value)(implementation, data);
}
Expand All @@ -93,7 +115,9 @@ contract BaseApp is Ownable {
* This is needed because only the proxy admin can query it.
* @return The address of the current implementation of the proxy.
*/
function getProxyImplementation(AdminUpgradeabilityProxy proxy) public view returns (address) {
function getProxyImplementation(AdminUpgradeabilityProxy proxy)
public view returns (address)
{
return proxy.implementation();
}

Expand All @@ -102,7 +126,16 @@ contract BaseApp is Ownable {
* Only the admin can query it.
* @return The address of the current admin of the proxy.
*/
function getProxyAdmin(AdminUpgradeabilityProxy proxy) public view returns (address) {
function getProxyAdmin(AdminUpgradeabilityProxy proxy)
public view returns (address)
{
return proxy.admin();
}

/**
* @dev Abstract function to return the implementation provider.
* @return The implementation provider.
*/
function getProvider() internal view returns (ImplementationProvider);

}
7 changes: 6 additions & 1 deletion contracts/application/PackagedApp.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import "./BaseApp.sol";
import "./versioning/Package.sol";
import "../upgradeability/UpgradeabilityProxyFactory.sol";


/**
* @title PackagedApp
* @dev App for an upgradeable project that can use different versions.
Expand All @@ -21,7 +22,11 @@ contract PackagedApp is BaseApp {
* @param _version Initial version of the app.
* @param _factory Proxy factory.
*/
function PackagedApp(Package _package, string _version, UpgradeabilityProxyFactory _factory)
function PackagedApp(
Package _package,
string _version,
UpgradeabilityProxyFactory _factory
)
BaseApp(_factory)
public
{
Expand Down
23 changes: 14 additions & 9 deletions contracts/application/UnversionedApp.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import "./BaseApp.sol";
import "./versioning/ImplementationProvider.sol";
import "../upgradeability/UpgradeabilityProxyFactory.sol";


/**
* @title UnversionedApp
* @dev Basic implementation of an upgradable app with no versioning.
Expand All @@ -19,21 +20,16 @@ contract UnversionedApp is BaseApp {
* @param _provider Implementation provider.
* @param _factory Proxy factory.
*/
function UnversionedApp(ImplementationProvider _provider, UpgradeabilityProxyFactory _factory)
function UnversionedApp(
ImplementationProvider _provider,
UpgradeabilityProxyFactory _factory
)
BaseApp(_factory)
public
{
setProvider(_provider);
}

/**
* @dev Returns the provider used by the app.
* @return The provider.
*/
function getProvider() internal view returns (ImplementationProvider) {
return provider;
}

/**
* @dev Sets a new implementation provider.
* @param _provider New implementation provider
Expand All @@ -42,4 +38,13 @@ contract UnversionedApp is BaseApp {
require(address(_provider) != address(0));
provider = _provider;
}

/**
* @dev Returns the provider used by the app.
* @return The provider.
*/
function getProvider() internal view returns (ImplementationProvider) {
return provider;
}

}
10 changes: 8 additions & 2 deletions contracts/application/versioning/FreezableImplementationDirectory.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,12 @@ pragma solidity ^0.4.21;

import "./ImplementationDirectory.sol";


/**
* @title FreezableImplementationDirectory
* @dev Implementation directory which can be made irreversibly immutable by the owner.
*/
contract FreezableImplementationDirectory is ImplementationDirectory {
contract FreezableImplementationDirectory is ImplementationDirectory {
/// @dev Mutability state of the directory.
bool public frozen;

Expand All @@ -32,7 +33,12 @@ import "./ImplementationDirectory.sol";
* @param contractName Name of the contract.
* @param implementation Address where the contract is implemented.
*/
function setImplementation(string contractName, address implementation) public whenNotFrozen {
function setImplementation(
string contractName,
address implementation
)
public whenNotFrozen
{
super.setImplementation(contractName, implementation);
}
}
15 changes: 12 additions & 3 deletions contracts/application/versioning/ImplementationDirectory.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@ pragma solidity ^0.4.21;

import "./ImplementationProvider.sol";
import "openzeppelin-solidity/contracts/ownership/Ownable.sol";
import 'openzeppelin-solidity/contracts/AddressUtils.sol';
import "openzeppelin-solidity/contracts/AddressUtils.sol";


/**
* @title ImplementationDirectory
Expand All @@ -24,7 +25,9 @@ contract ImplementationDirectory is ImplementationProvider, Ownable {
* @param contractName Name of the contract.
* @return Address of the implementation.
*/
function getImplementation(string contractName) public view returns (address) {
function getImplementation(string contractName)
public view returns (address)
{
return implementations[contractName];
}

Expand All @@ -33,7 +36,13 @@ contract ImplementationDirectory is ImplementationProvider, Ownable {
* @param contractName Name of the contract.
* @param implementation Address of the implementation.
*/
function setImplementation(string contractName, address implementation) public onlyOwner {
function setImplementation(
string contractName,
address implementation
)
public
onlyOwner
{
require(AddressUtils.isContract(implementation));
implementations[contractName] = implementation;
emit ImplementationChanged(contractName, implementation);
Expand Down
3 changes: 2 additions & 1 deletion contracts/application/versioning/ImplementationProvider.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,6 @@ interface ImplementationProvider {
* @param contractName Name of the contract.
* @return Implementation address of the contract.
*/
function getImplementation(string contractName) public view returns (address);
function getImplementation(string contractName)
public view returns (address);
}
Loading