Solidity and JS linters #328

nkuba · 2020-03-26T23:14:46Z

In this PR we:

configured linters for Solidity and JavaScript code,
added a pre-commit hook to enforce correct code formatting,
added CircleCI job to execute linters on git push
fixed formating of JS and Solidity files.

Closes #42

Shadowfiend · 2020-03-26T23:19:20Z

.circleci/config.yml

+      - run:
+          name: Install dependencies
+          working_directory: ~/project/solidity
+          command: npm install


npm ci is the jam here.

Shadowfiend · 2020-03-26T23:19:37Z

.circleci/config.yml

+  lint:
+    jobs:
+      - setup_github_package_registry:
+          context: github-package-registry


You probably don't need this anymore since the repo is open, though not certain about that.

Nope, Unable to authenticate, need: Basic realm="GitHub Package Registry" 😞

I think this happened for a couple of reasons:

You need to update package-lock by running npm i.

You need to drop .npmrc.

You need to drop the publishConfig.

However, I'll do all of this stuff in one step when I handle deployment later today.

Shadowfiend

Few comments around the prettier config for JS.

Shadowfiend · 2020-03-26T23:20:10Z

solidity/.prettierrc.yaml

@@ -0,0 +1,2 @@
+semi: false
+trailingComma: "all"


We shouldn't have a custom .prettierrc.

Nice! I just copy-pasted it from tbtc 😉

Yeah, it predates my adding prettier to eslint and I haven't migrated it yet 😬

Shadowfiend · 2020-03-26T23:20:33Z

solidity/.soliumrc.json

+    "security/no-call-value": "off",
+    "security/no-inline-assembly": "off"
+  }


We should consider unifying these rules at the keep level, as a side note. Haven't peeked into this yet, so not a blocking note or anything.

Shadowfiend · 2020-03-26T23:21:01Z

solidity/contracts/BondedECDSAKeep.sol

@@ -1,3 +1,4 @@
+/* solium-disable function-order */


Any particular reason for this? I would kind of rather just go with the function order in our linter. Fewer toggles is better.

We tried to place the functions in a logical order as they may be used in the flow. But yeah, sure I can rearrange the functions order to satisfy solium.

I actually get this and it tends to be my first preference during initial dev, but there are some strong arguments for function ordering for visibility. Diligence also flagged this possibility. My main thing is I'd rather either go all in on ignoring this, or not ignore it ever heh. I'm already twitchy over the other exceptions we have to solium.

As I like the idea to conform to the rules and order the functions accordingly, there is one place where IMO it would be better to disable the check.
It's in BondedECDSAKeepVendor contract where we have a long list of getters/setters pairs where getters are public and setters internal:

keep-ecdsa/solidity/contracts/BondedECDSAKeepVendor.sol

Lines 158 to 260 in d7bf095

/// @dev Returns the current implementation. Implements function from `Proxy`

/// contract.

/// @return Address of the current implementation

function _implementation() internal view returns (address impl) {

bytes32 slot = IMPLEMENTATION_SLOT;

/* solium-disable-next-line */

assembly {

impl := sload(slot)

}

}

/// @notice Sets the address of the current implementation.

/// @param _implementation Address representing the new implementation to

/// be set.

function setImplementation(address _implementation) internal {

bytes32 slot = IMPLEMENTATION_SLOT;

/* solium-disable-next-line */

assembly {

sstore(slot, _implementation)

}

}

function upgradeTimeDelay()

public

view

returns (uint256 _upgradeTimeDelay)

{

bytes32 position = UPGRADE_TIME_DELAY_SLOT;

/* solium-disable-next-line */

assembly {

_upgradeTimeDelay := sload(position)

}

}

function setUpgradeTimeDelay(uint256 _upgradeTimeDelay) internal {

bytes32 position = UPGRADE_TIME_DELAY_SLOT;

/* solium-disable-next-line */

assembly {

sstore(position, _upgradeTimeDelay)

}

}

function newImplementation()

public

view

returns (address _newImplementation)

{

bytes32 position = UPGRADE_IMPLEMENTATION_SLOT;

/* solium-disable-next-line */

assembly {

_newImplementation := sload(position)

}

}

function setNewImplementation(address _newImplementation) internal {

bytes32 position = UPGRADE_IMPLEMENTATION_SLOT;

/* solium-disable-next-line */

assembly {

sstore(position, _newImplementation)

}

}

function upgradeInitiatedTimestamp()

public

view

returns (uint256 _upgradeInitiatedTimestamp)

{

bytes32 position = UPGRADE_INIT_TIMESTAMP_SLOT;

/* solium-disable-next-line */

assembly {

_upgradeInitiatedTimestamp := sload(position)

}

}

function setUpgradeInitiatedTimestamp(uint256 _upgradeInitiatedTimestamp)

internal

{

bytes32 position = UPGRADE_INIT_TIMESTAMP_SLOT;

/* solium-disable-next-line */

assembly {

sstore(position, _upgradeInitiatedTimestamp)

}

}

/// @notice The admin slot.

/// @return The contract owner's address.

function admin() public view returns (address adm) {

bytes32 slot = ADMIN_SLOT;

/* solium-disable-next-line */

assembly {

adm := sload(slot)

}

}

/// @dev Sets the address of the proxy admin.

/// @param _newAdmin Address of the new proxy admin.

function setAdmin(address _newAdmin) internal {

bytes32 slot = ADMIN_SLOT;

/* solium-disable-next-line */

assembly {

sstore(slot, _newAdmin)

}

}

Shadowfiend · 2020-03-26T23:22:46Z

solidity/package.json

@@ -43,7 +52,14 @@
    "@truffle/hdwallet-provider": "^1.0.25",
    "bn-chai": "^1.0.1",
    "chai": "^4.2.0",
+    "eslint": "^6.8.0",
+    "eslint-config-keep": "github:keep-network/eslint-config-keep",


Let's anchor this to a version tag.

Shadowfiend · 2020-03-26T23:25:32Z

solidity/package.json

@@ -43,7 +52,14 @@
    "@truffle/hdwallet-provider": "^1.0.25",
    "bn-chai": "^1.0.1",
    "chai": "^4.2.0",
+    "eslint": "^6.8.0",
+    "eslint-config-keep": "github:keep-network/eslint-config-keep",
+    "eslint-config-prettier": "^6.10.1",


Burn it please :) eslint-config-keep should handle this. I realize it doesn't currently deal with the Solidity plugin, but we still need to explicitly include the prettier dependency here anyway.

My suggestion:

Kill eslint-config-prettier. Let eslint-config-keep#0.3.0 handle the JS side of prettier. You still need the prettier dependency below for this, but you lose the explicit prettier invocation for lint:js and lint:fix:js.

Continue to pull in prettier-plugin-solidity and run prettier in the :sol scripts.

I also want to see if I can pull prettier into solium's config, but that seems unlikely. There may be room to set something up with our core eslint config and Solidity though, TBD.

nkuba · 2020-03-27T00:24:49Z

solidity/contracts/BondedECDSAKeep.sol

+    /// @param _keepBonding Address of the KeepBonding contract.
+    /// @param _keepFactory Address of the BondedECDSAKeepFactory that created
+    /// this keep.
+    function initialize(


Moving this function below external ones hurts me because this is like a constructor that we use in the factory pattern.

We no longer need it as required repos are public

This reverts commit c453285.

It is included in eslint-config-keep#0.3.0

Shadowfiend · 2020-03-30T15:04:47Z

solidity/migrations/external-contracts.js

+const RegistryAddress = "0x9aF7d664E6d7DB318c6BdE54ED77A7D99361F539"
+const TokenStakingAddress = "0x602c5f7d11eb786E14cDfBafe34E70c0b6717a8c"
+const RandomBeaconAddress = "0xB901EDDE70EcB59c1243f404a238fd97B78f1F54"
+const TBTCSystemAddress = "0x065c3Ab1D2cb67c70dAC17Cf3BD4B8A165ffceFa"


Is this on purpose? <_<

Oh boy, no! It's fixed in 43bfb27.
Thanks for noticing that!

Shadowfiend

Ok… Let's do this crazy thing.

nkuba requested a review from Shadowfiend March 26, 2020 23:16

nkuba mentioned this pull request Mar 26, 2020

Linter integration #44

Closed

Shadowfiend reviewed Mar 26, 2020

View reviewed changes

nkuba requested a review from Shadowfiend March 26, 2020 23:33

nkuba commented Mar 27, 2020

View reviewed changes

nkuba added 15 commits March 27, 2020 11:26

Linters configuration

98cec5c

JS files formatting update

e646faf

Added solium dependency

da9b780

Added solitidy linting to pre commit hook

0d6bdff

Solidity contracts linting

2d09376

Added linting as circle ci job

f8aa0a2

Added step to install dependencies

0ab36db

Added dependency to auth step

e5e3e41

Replaced npm install with npm ci

8562d65

Removed github registry authentication

fa633f3

We no longer need it as required repos are public

Revert "Removed github registry authentication"

48abe79

This reverts commit c453285.

Pin dependencies versions

44e5571

Removed prettier custom config and updated code formatting

f7ca9cc

Removec eslint-config-prettier dependency

128a650

It is included in eslint-config-keep#0.3.0

Updated function order

5e7de6c

nkuba force-pushed the linting branch from d473c7d to 5e7de6c Compare March 27, 2020 10:37

Shadowfiend reviewed Mar 30, 2020

View reviewed changes

Shadowfiend approved these changes Mar 30, 2020

View reviewed changes

Merge branch 'master' into linting

28c85a4

Shadowfiend merged commit 8f22080 into master Mar 30, 2020

Shadowfiend deleted the linting branch March 30, 2020 18:35

pdyraga added this to the v0.11.0 milestone Mar 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Solidity and JS linters #328

Solidity and JS linters #328

nkuba commented Mar 26, 2020

Shadowfiend Mar 26, 2020

Shadowfiend Mar 26, 2020

nkuba Mar 26, 2020

Shadowfiend Mar 30, 2020

Shadowfiend left a comment

Shadowfiend Mar 26, 2020

nkuba Mar 26, 2020

Shadowfiend Mar 27, 2020

Shadowfiend Mar 26, 2020

Shadowfiend Mar 26, 2020

nkuba Mar 26, 2020

Shadowfiend Mar 27, 2020

nkuba Mar 27, 2020

Shadowfiend Mar 26, 2020

Shadowfiend Mar 26, 2020

nkuba Mar 27, 2020

Shadowfiend Mar 30, 2020

nkuba Mar 30, 2020

Shadowfiend left a comment

	/// @dev Returns the current implementation. Implements function from `Proxy`
	/// contract.
	/// @return Address of the current implementation
	function _implementation() internal view returns (address impl) {
	bytes32 slot = IMPLEMENTATION_SLOT;
	/* solium-disable-next-line */
	assembly {
	impl := sload(slot)
	}
	}

	/// @notice Sets the address of the current implementation.
	/// @param _implementation Address representing the new implementation to
	/// be set.
	function setImplementation(address _implementation) internal {
	bytes32 slot = IMPLEMENTATION_SLOT;
	/* solium-disable-next-line */
	assembly {
	sstore(slot, _implementation)
	}
	}

	function upgradeTimeDelay()
	public
	view
	returns (uint256 _upgradeTimeDelay)
	{
	bytes32 position = UPGRADE_TIME_DELAY_SLOT;
	/* solium-disable-next-line */
	assembly {
	_upgradeTimeDelay := sload(position)
	}
	}

	function setUpgradeTimeDelay(uint256 _upgradeTimeDelay) internal {
	bytes32 position = UPGRADE_TIME_DELAY_SLOT;
	/* solium-disable-next-line */
	assembly {
	sstore(position, _upgradeTimeDelay)
	}
	}

	function newImplementation()
	public
	view
	returns (address _newImplementation)
	{
	bytes32 position = UPGRADE_IMPLEMENTATION_SLOT;
	/* solium-disable-next-line */
	assembly {
	_newImplementation := sload(position)
	}
	}

	function setNewImplementation(address _newImplementation) internal {
	bytes32 position = UPGRADE_IMPLEMENTATION_SLOT;
	/* solium-disable-next-line */
	assembly {
	sstore(position, _newImplementation)
	}
	}

	function upgradeInitiatedTimestamp()
	public
	view
	returns (uint256 _upgradeInitiatedTimestamp)
	{
	bytes32 position = UPGRADE_INIT_TIMESTAMP_SLOT;
	/* solium-disable-next-line */
	assembly {
	_upgradeInitiatedTimestamp := sload(position)
	}
	}

	function setUpgradeInitiatedTimestamp(uint256 _upgradeInitiatedTimestamp)
	internal
	{
	bytes32 position = UPGRADE_INIT_TIMESTAMP_SLOT;
	/* solium-disable-next-line */
	assembly {
	sstore(position, _upgradeInitiatedTimestamp)
	}
	}

	/// @notice The admin slot.
	/// @return The contract owner's address.
	function admin() public view returns (address adm) {
	bytes32 slot = ADMIN_SLOT;
	/* solium-disable-next-line */
	assembly {
	adm := sload(slot)
	}
	}

	/// @dev Sets the address of the proxy admin.
	/// @param _newAdmin Address of the new proxy admin.
	function setAdmin(address _newAdmin) internal {
	bytes32 slot = ADMIN_SLOT;
	/* solium-disable-next-line */
	assembly {
	sstore(slot, _newAdmin)
	}
	}

Solidity and JS linters #328

Solidity and JS linters #328

Conversation

nkuba commented Mar 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shadowfiend left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shadowfiend left a comment

Choose a reason for hiding this comment