From 97a4fdfcc001bc48f66f5b50a724eaafc2e8f9b6 Mon Sep 17 00:00:00 2001 From: dbale-altoros Date: Fri, 27 Jan 2023 12:20:32 -0300 Subject: [PATCH 1/3] ordering-rule custom error support --- lib/rules/order/ordering.js | 4 ++ test/rules/order/ordering.js | 106 ++++++++++++++++++++++++++++++++++- 2 files changed, 109 insertions(+), 1 deletion(-) diff --git a/lib/rules/order/ordering.js b/lib/rules/order/ordering.js index 9c77e557..5cb3fae2 100644 --- a/lib/rules/order/ordering.js +++ b/lib/rules/order/ordering.js @@ -135,6 +135,10 @@ function contractPartOrder(node) { return [30, 'event definition'] } + if (node.type === 'CustomErrorDefinition') { + return [35, 'custom error definition'] + } + if (node.type === 'ModifierDefinition') { return [40, 'modifier definition'] } diff --git a/test/rules/order/ordering.js b/test/rules/order/ordering.js index d80fc808..7e65ecc4 100644 --- a/test/rules/order/ordering.js +++ b/test/rules/order/ordering.js @@ -145,7 +145,7 @@ describe('Linter - ordering', () => { assert.ok(report.messages[0].message.includes('Function order is incorrect')) }) - it('should not raise error when external const goes before public ', () => { + it('should not raise error when external function goes before public ', () => { const code = contractWith(` function a() external view {} function b() public {} @@ -158,6 +158,110 @@ describe('Linter - ordering', () => { assert.equal(report.errorCount, 0) }) + it('should raise an error when custom error is after external function', () => { + const code = contractWith(` + function a() external view {} + error Unauthorized(); + function b() public {} + `) + + const report = linter.processStr(code, { + rules: { ordering: 'error' }, + }) + + assert.equal(report.errorCount, 1) + assert.ok( + report.messages[0].message.includes( + 'Function order is incorrect, custom error definition can not go after external view function' + ) + ) + }) + + it('should raise an error when custom error is after public function', () => { + const code = contractWith(` + function b() public {} + error Unauthorized(); + function a() external view {} + `) + + const report = linter.processStr(code, { + rules: { ordering: 'error' }, + }) + + assert.equal(report.errorCount, 1) + assert.ok( + report.messages[0].message.includes( + 'Function order is incorrect, custom error definition can not go after public function' + ) + ) + }) + + it('should raise an error when custom error is after constructor', () => { + const code = contractWith(` + constructor() {} + error Unauthorized(); + `) + + const report = linter.processStr(code, { + rules: { ordering: 'error' }, + }) + + assert.equal(report.errorCount, 1) + assert.ok( + report.messages[0].message.includes( + 'Function order is incorrect, custom error definition can not go after constructor' + ) + ) + }) + + it('should raise an error when custom error is before event definition', () => { + const code = contractWith(` + error Unauthorized(); + event WithdrawRegistered(uint256 receiver); + constructor() {} + `) + + const report = linter.processStr(code, { + rules: { ordering: 'error' }, + }) + + assert.equal(report.errorCount, 1) + assert.ok( + report.messages[0].message.includes( + 'Function order is incorrect, event definition can not go after custom error' + ) + ) + }) + + it('should not raise an error when custom error is well placed after event and before modifier', () => { + const code = contractWith(` + event WithdrawRegistered(uint256 receiver); + error Unauthorized(); + modifier onlyOwner() {} + constructor() {} + `) + + const report = linter.processStr(code, { + rules: { ordering: 'error' }, + }) + + assert.equal(report.errorCount, 0) + }) + + it('should not raise an error when custom error is well placed after state variables and before constructor', () => { + const code = contractWith(` + uint256 public stateVariable; + error Unauthorized(); + constructor() {} + `) + + const report = linter.processStr(code, { + rules: { ordering: 'error' }, + }) + + assert.equal(report.errorCount, 0) + }) + it('should raise error for enum after contract', () => { const code = ` contract Foo {} From 0b4b278f72d9b18299b3837f0f855c51ca126966 Mon Sep 17 00:00:00 2001 From: dbale-altoros Date: Tue, 14 Feb 2023 18:25:18 -0300 Subject: [PATCH 2/3] ordering rule: added support for file level statements --- lib/rules/order/ordering.js | 13 +++- test/rules/order/ordering.js | 138 +++++++++++++++++++++++++++++++++++ 2 files changed, 150 insertions(+), 1 deletion(-) diff --git a/lib/rules/order/ordering.js b/lib/rules/order/ordering.js index 5cb3fae2..5de2b017 100644 --- a/lib/rules/order/ordering.js +++ b/lib/rules/order/ordering.js @@ -28,7 +28,6 @@ class OrderingChecker extends BaseChecker { SourceUnit(node) { const children = node.children - this.checkOrder(children, sourceUnitPartOrder) } @@ -90,10 +89,22 @@ function sourceUnitPartOrder(node) { return [10, 'import directive'] } + if (node.type === 'CustomErrorDefinition') { + return [15, 'custom error definition'] + } + if (node.type === 'EnumDefinition' || node.type === 'StructDefinition') { return [20, 'type definition'] } + if (node.type === 'FileLevelConstant') { + return [23, 'file level constant'] + } + + if (node.type === 'FunctionDefinition') { + return [26, 'free function definition'] + } + if (node.type === 'ContractDefinition') { if (node.kind === 'interface') { return [30, 'interface'] diff --git a/test/rules/order/ordering.js b/test/rules/order/ordering.js index 7e65ecc4..1081a302 100644 --- a/test/rules/order/ordering.js +++ b/test/rules/order/ordering.js @@ -289,4 +289,142 @@ describe('Linter - ordering', () => { assert.equal(report.errorCount, 1) }) + + it('should raise error when custom error is before import', () => { + const code = ` + // SPDX-License-Identifier: MIT + pragma solidity ^0.8.0; + error Unauthorized(); + import "@openzeppelin/contracts/ownership/Ownable.sol"; + contract OneNiceContract {} + ` + const report = linter.processStr(code, { + rules: { ordering: 'error' }, + }) + + assert.equal(report.errorCount, 1) + assert.ok( + report.messages[0].message.includes( + 'Function order is incorrect, import directive can not go after custom error definition' + ) + ) + }) + + it('should not raise error when custom error is defined in correct order', () => { + const code = ` + // SPDX-License-Identifier: MIT + pragma solidity ^0.8.0; + import "@openzeppelin/contracts/ownership/Ownable.sol"; + error Unauthorized(); + contract OneNiceContract {} + ` + const report = linter.processStr(code, { + rules: { ordering: 'error' }, + }) + + assert.equal(report.errorCount, 0) + }) + + it('should raise error when free function is before custom error', () => { + const code = ` + // SPDX-License-Identifier: MIT + pragma solidity ^0.8.0; + import "@openzeppelin/contracts/ownership/Ownable.sol"; + function freeFunction() pure returns (uint256) { + return 1; + } + error Unauthorized(); + contract OneNiceContract {} + ` + const report = linter.processStr(code, { + rules: { ordering: 'error' }, + }) + + assert.equal(report.errorCount, 1) + assert.ok( + report.messages[0].message.includes( + 'Function order is incorrect, custom error definition can not go after free function definition' + ) + ) + }) + + it('should not raise error when free function is defined in correct order', () => { + const code = ` + // SPDX-License-Identifier: MIT + pragma solidity ^0.8.0; + import "@openzeppelin/contracts/ownership/Ownable.sol"; + error Unauthorized(); + function freeFunction() pure returns (uint256) { + return 1; + } + contract OneNiceContract {} + ` + const report = linter.processStr(code, { + rules: { ordering: 'error' }, + }) + + assert.equal(report.errorCount, 0) + }) + + it('should raise error when file level constant is defined after free function', () => { + const code = ` + // SPDX-License-Identifier: MIT + pragma solidity ^0.8.0; + import "@openzeppelin/contracts/ownership/Ownable.sol"; + function freeFunction() pure returns (uint256) { + return 1; + } + uint256 constant oneNiceConstant = 1; + contract OneNiceContract {} + ` + const report = linter.processStr(code, { + rules: { ordering: 'error' }, + }) + + assert.equal(report.errorCount, 1) + assert.ok( + report.messages[0].message.includes( + 'Function order is incorrect, file level constant can not go after free function definition' + ) + ) + }) + + it('should not raise error when file level constant is defined in correct order', () => { + const code = ` + // SPDX-License-Identifier: MIT + pragma solidity ^0.8.0; + import "@openzeppelin/contracts/ownership/Ownable.sol"; + uint256 constant oneNiceConstant = 1; + function freeFunction() pure returns (uint256) { + return 1; + } + contract OneNiceContract {} + ` + const report = linter.processStr(code, { + rules: { ordering: 'error' }, + }) + + assert.equal(report.errorCount, 0) + }) + + it('should not raise error when all top level code is well placed', () => { + const code = ` + // SPDX-License-Identifier: MIT + pragma solidity ^0.8.0; + import "@openzeppelin/contracts/ownership/Ownable.sol"; + error Unauthorized(); + enum MyEnum { A, B } + struct OneNiceStruct { uint256 a; uint256 b; } + uint256 constant oneNiceConstant = 1; + function freeFunction() pure returns (uint256) { + return 1; + } + contract OneNiceContract {} + ` + const report = linter.processStr(code, { + rules: { ordering: 'error' }, + }) + + assert.equal(report.errorCount, 0) + }) }) From e57a6802aeb81dfcd64b3ca97ce61baf6226c583 Mon Sep 17 00:00:00 2001 From: dbale-altoros Date: Tue, 21 Feb 2023 10:39:20 -0300 Subject: [PATCH 3/3] fix: changed weight order --- lib/rules/order/ordering.js | 18 +++++++++--------- test/rules/order/ordering.js | 4 ++-- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/lib/rules/order/ordering.js b/lib/rules/order/ordering.js index 5de2b017..19c731f6 100644 --- a/lib/rules/order/ordering.js +++ b/lib/rules/order/ordering.js @@ -89,33 +89,33 @@ function sourceUnitPartOrder(node) { return [10, 'import directive'] } - if (node.type === 'CustomErrorDefinition') { - return [15, 'custom error definition'] + if (node.type === 'FileLevelConstant') { + return [20, 'file level constant'] } if (node.type === 'EnumDefinition' || node.type === 'StructDefinition') { - return [20, 'type definition'] + return [30, 'type definition'] } - if (node.type === 'FileLevelConstant') { - return [23, 'file level constant'] + if (node.type === 'CustomErrorDefinition') { + return [40, 'custom error definition'] } if (node.type === 'FunctionDefinition') { - return [26, 'free function definition'] + return [50, 'free function definition'] } if (node.type === 'ContractDefinition') { if (node.kind === 'interface') { - return [30, 'interface'] + return [60, 'interface'] } if (node.kind === 'library') { - return [40, 'library definition'] + return [70, 'library definition'] } if (node.kind === 'contract') { - return [50, 'contract definition'] + return [80, 'contract definition'] } } diff --git a/test/rules/order/ordering.js b/test/rules/order/ordering.js index 1081a302..d83d73f4 100644 --- a/test/rules/order/ordering.js +++ b/test/rules/order/ordering.js @@ -412,10 +412,10 @@ describe('Linter - ordering', () => { // SPDX-License-Identifier: MIT pragma solidity ^0.8.0; import "@openzeppelin/contracts/ownership/Ownable.sol"; - error Unauthorized(); + uint256 constant oneNiceConstant = 1; enum MyEnum { A, B } struct OneNiceStruct { uint256 a; uint256 b; } - uint256 constant oneNiceConstant = 1; + error Unauthorized(); function freeFunction() pure returns (uint256) { return 1; }