Skip to content

Commit

Permalink
Merge pull request #555 from protofire/fix-custom-error-check-sol-ver…
Browse files Browse the repository at this point in the history 
…sion

fix: custom errors checks from 0.8.4 forward
  • Loading branch information
@dbale-altoros
dbale-altoros authored Mar 6, 2024
2 parents 02d94a8 + e1ef82d commit 30846a6
Show file tree
Hide file tree
Showing 3 changed files with 119 additions and 19 deletions.
2 changes: 2 additions & 0 deletions docs/rules/gas-consumption/gas-custom-errors.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ This rule accepts a string option of rule severity. Must be one of "error", "war
}
```

### Notes
- This rules applies to Solidity version 0.8.4 and higher

## Examples
### 👍 Examples of **correct** code for this rule
Expand Down
66 changes: 55 additions & 11 deletions lib/rules/gas-consumption/gas-custom-errors.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ const BaseChecker = require('../base-checker')
const { severityDescription } = require('../../doc/utils')

const DEFAULT_SEVERITY = 'warn'

const BASE_VERSION = [0, 8, 4]
const ruleId = 'gas-custom-errors'
const meta = {
type: 'gas-consumption',
Expand All @@ -16,6 +16,11 @@ const meta = {
default: DEFAULT_SEVERITY,
},
],
notes: [
{
note: 'This rules applies to Solidity version 0.8.4 and higher',
},
],
examples: {
good: [
{
Expand Down Expand Up @@ -54,21 +59,60 @@ const meta = {
class GasCustomErrorsChecker extends BaseChecker {
constructor(reporter) {
super(reporter, ruleId, meta)
this.solidityVersion = 0
}

parseVersion(version) {
// Remove any leading ^ or ~ and other non-numeric characters before the first digit
const cleanVersion = version.replace(/^[^\d]+/, '')
return cleanVersion.split('.').map((num) => parseInt(num, 10))
}

isVersionGreater(node) {
const currentVersion = this.parseVersion(this.solidityVersion)
if (currentVersion.length !== 3) {
this.error(node, `GC: Invalid Solidity version`)
return
}

for (let i = 0; i < BASE_VERSION.length; i++) {
if (currentVersion[i] < BASE_VERSION[i]) {
// If any part of the current version is less than the base version, return false
return false
} else if (currentVersion[i] > BASE_VERSION[i]) {
// If any part of the current version is greater, no need to check further, return true
return true
}
// If parts are equal, continue to the next part
}

// Reaching here means all parts compared are equal, or the base version parts have been exhausted,
// so the current version is at least equal, return true
return true
}

PragmaDirective(node) {
if (node.type === 'PragmaDirective' && node.name === 'solidity') {
this.solidityVersion = node.value
}
}

FunctionCall(node) {
let errorStr = ''
if (node.expression.name === 'require') {
errorStr = 'require'
} else if (
node.expression.name === 'revert' &&
(node.arguments.length === 0 || node.arguments[0].type === 'StringLiteral')
) {
errorStr = 'revert'
}

if (errorStr !== '') {
this.error(node, `GC: Use Custom Errors instead of ${errorStr} statements`)
if (this.isVersionGreater(node)) {
if (node.expression.name === 'require') {
errorStr = 'require'
} else if (
node.expression.name === 'revert' &&
(node.arguments.length === 0 || node.arguments[0].type === 'StringLiteral')
) {
errorStr = 'revert'
}

if (errorStr !== '') {
this.error(node, `GC: Use Custom Errors instead of ${errorStr} statements`)
}
}
}
}
Expand Down
70 changes: 62 additions & 8 deletions test/rules/gas-consumption/gas-custom-errors.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,18 @@ const {
const linter = require('../../../lib/index')
const { funcWith } = require('../../common/contract-builder')

function replaceSolidityVersion(code, newVersion) {
// Regular expression to match the version number in the pragma directive
const versionRegex = /pragma solidity \d+\.\d+\.\d+;/
// Replace the matched version with the newVersion
return code.replace(versionRegex, `pragma solidity ${newVersion};`)
}

describe('Linter - gas-custom-errors', () => {
it('should raise error for revert()', () => {
const code = funcWith(`revert();`)
let code = funcWith(`revert();`)
code = replaceSolidityVersion(code, '^0.8.4')

const report = linter.processStr(code, {
rules: { 'gas-custom-errors': 'error' },
})
Expand All @@ -21,7 +30,9 @@ describe('Linter - gas-custom-errors', () => {
})

it('should raise error for revert([string])', () => {
const code = funcWith(`revert("Insufficent funds");`)
let code = funcWith(`revert("Insufficent funds");`)
code = replaceSolidityVersion(code, '0.8.4')

const report = linter.processStr(code, {
rules: { 'gas-custom-errors': 'error' },
})
Expand All @@ -31,7 +42,9 @@ describe('Linter - gas-custom-errors', () => {
})

it('should NOT raise error for revert ErrorFunction()', () => {
const code = funcWith(`revert ErrorFunction();`)
let code = funcWith(`revert ErrorFunction();`)
code = replaceSolidityVersion(code, '^0.8.22')

const report = linter.processStr(code, {
rules: { 'gas-custom-errors': 'error' },
})
Expand All @@ -41,7 +54,9 @@ describe('Linter - gas-custom-errors', () => {
})

it('should NOT raise error for revert ErrorFunction() with arguments', () => {
const code = funcWith(`revert ErrorFunction({ msg: "Insufficent funds msg" });`)
let code = funcWith(`revert ErrorFunction({ msg: "Insufficent funds msg" });`)
code = replaceSolidityVersion(code, '^0.8.5')

const report = linter.processStr(code, {
rules: { 'gas-custom-errors': 'error' },
})
Expand All @@ -51,9 +66,11 @@ describe('Linter - gas-custom-errors', () => {
})

it('should raise error for require', () => {
const code = funcWith(`require(!has(role, account), "Roles: account already has role");
let code = funcWith(`require(!has(role, account), "Roles: account already has role");
role.bearer[account] = true;role.bearer[account] = true;
`)
code = replaceSolidityVersion(code, '0.8.21')

const report = linter.processStr(code, {
rules: { 'gas-custom-errors': 'error' },
})
Expand All @@ -63,9 +80,11 @@ describe('Linter - gas-custom-errors', () => {
})

it('should NOT raise error for regular function call', () => {
const code = funcWith(`callOtherFunction();
let code = funcWith(`callOtherFunction();
role.bearer[account] = true;role.bearer[account] = true;
`)
code = replaceSolidityVersion(code, '^0.9.0')

const report = linter.processStr(code, {
rules: { 'gas-custom-errors': 'error' },
})
Expand All @@ -74,10 +93,12 @@ describe('Linter - gas-custom-errors', () => {
})

it('should raise error for require, revert message and not for revert CustomError() for [recommended] config', () => {
const code = funcWith(`require(!has(role, account), "Roles: account already has role");
let code = funcWith(`require(!has(role, account), "Roles: account already has role");
revert("RevertMessage");
revert CustomError();
`)
code = replaceSolidityVersion(code, '0.8.20')

const report = linter.processStr(code, {
extends: 'solhint:recommended',
rules: { 'compiler-version': 'off' },
Expand All @@ -88,7 +109,7 @@ describe('Linter - gas-custom-errors', () => {
assert.equal(report.reports[1].message, 'GC: Use Custom Errors instead of revert statements')
})

it('should NOT raise error for default config', () => {
it('should NOT raise error for default config because rule is not part of default', () => {
const code = funcWith(`require(!has(role, account), "Roles: account already has role");
revert("RevertMessage");
revert CustomError();
Expand All @@ -100,4 +121,37 @@ describe('Linter - gas-custom-errors', () => {
assertWarnsCount(report, 0)
assertErrorCount(report, 0)
})

it('should NOT raise error for lower versions 0.8.3', () => {
let code = funcWith(`revert();`)
code = replaceSolidityVersion(code, '0.8.3')

const report = linter.processStr(code, {
rules: { 'gas-custom-errors': 'error' },
})

assertErrorCount(report, 0)
})

it('should NOT raise error for lower versions 0.4.4', () => {
const code = funcWith(`revert("Insufficent funds");`)

const report = linter.processStr(code, {
rules: { 'gas-custom-errors': 'error' },
})

assertErrorCount(report, 0)
})

it('should NOT raise error for lower versions 0.8.3', () => {
let code = funcWith(`revert();`)
code = replaceSolidityVersion(code, '0.8')

const report = linter.processStr(code, {
rules: { 'gas-custom-errors': 'error' },
})

assertErrorCount(report, 1)
assertErrorMessage(report, 'GC: Invalid Solidity version')
})
})

0 comments on commit 30846a6

Please sign in to comment.