From 2099883d64adcb017866a8613b9d70f6ca4ac86d Mon Sep 17 00:00:00 2001 From: dbale-altoros Date: Thu, 18 Jan 2024 16:21:26 -0300 Subject: [PATCH 1/2] gas: calldata parameters --- conf/rulesets/solhint-all.js | 1 + docs/rules.md | 11 +- .../gas-calldata-parameters.md | 41 +++++++ .../gas-calldata-parameters.js | 111 +++++++++++++++++ lib/rules/gas-consumption/index.js | 3 + .../gas-calldata-parameters.js | 113 ++++++++++++++++++ 6 files changed, 275 insertions(+), 5 deletions(-) create mode 100644 docs/rules/gas-consumption/gas-calldata-parameters.md create mode 100644 lib/rules/gas-consumption/gas-calldata-parameters.js create mode 100644 test/rules/gas-consumption/gas-calldata-parameters.js diff --git a/conf/rulesets/solhint-all.js b/conf/rulesets/solhint-all.js index 79c9a8ee..9b664a8c 100644 --- a/conf/rulesets/solhint-all.js +++ b/conf/rulesets/solhint-all.js @@ -25,6 +25,7 @@ module.exports = Object.freeze({ }, ], 'constructor-syntax': 'warn', + 'gas-calldata-parameters': 'warn', 'gas-indexed-events': 'warn', 'gas-multitoken1155': 'warn', 'gas-small-strings': 'warn', diff --git a/docs/rules.md b/docs/rules.md index c12bdf54..6f6cfcb4 100644 --- a/docs/rules.md +++ b/docs/rules.md @@ -27,11 +27,12 @@ title: "Rule Index of Solhint" ## Gas Consumption Rules -| Rule Id | Error | Recommended | Deprecated | -| ------------------------------------------------------------------- | -------------------------------------------------------------- | ----------- | ---------- | -| [gas-indexed-events](./rules/gas-consumption/gas-indexed-events.md) | Suggest indexed arguments on events for uint, bool and address | | | -| [gas-multitoken1155](./rules/gas-consumption/gas-multitoken1155.md) | ERC1155 is a cheaper non-fungible token than ERC721 | | | -| [gas-small-strings](./rules/gas-consumption/gas-small-strings.md) | Keep strings smaller than 32 bytes | | | +| Rule Id | Error | Recommended | Deprecated | +| ----------------------------------------------------------------------------- | -------------------------------------------------------------- | ----------- | ---------- | +| [gas-calldata-parameters](./rules/gas-consumption/gas-calldata-parameters.md) | Suggest calldata keyword on function arguments when read only | | | +| [gas-indexed-events](./rules/gas-consumption/gas-indexed-events.md) | Suggest indexed arguments on events for uint, bool and address | | | +| [gas-multitoken1155](./rules/gas-consumption/gas-multitoken1155.md) | ERC1155 is a cheaper non-fungible token than ERC721 | | | +| [gas-small-strings](./rules/gas-consumption/gas-small-strings.md) | Keep strings smaller than 32 bytes | | | ## Miscellaneous diff --git a/docs/rules/gas-consumption/gas-calldata-parameters.md b/docs/rules/gas-consumption/gas-calldata-parameters.md new file mode 100644 index 00000000..907d514e --- /dev/null +++ b/docs/rules/gas-consumption/gas-calldata-parameters.md @@ -0,0 +1,41 @@ +--- +warning: "This is a dynamically generated file. Do not edit manually." +layout: "default" +title: "gas-calldata-parameters | Solhint" +--- + +# gas-calldata-parameters +![Category Badge](https://img.shields.io/badge/-Gas%20Consumption%20Rules-informational) +![Default Severity Badge warn](https://img.shields.io/badge/Default%20Severity-warn-yellow) + +## Description +Suggest calldata keyword on function arguments when read only + +## Options +This rule accepts a string option of rule severity. Must be one of "error", "warn", "off". Default to warn. + +### Example Config +```json +{ + "rules": { + "gas-calldata-parameters": "warn" + } +} +``` + +### Notes +- Only applies for external functions when receiving arguments with [memory] keyword +- This rule makes a soft check to see if argument is readOnly to make the suggestion. Check it manually before changing it. +- [source 1](https://coinsbench.com/comprehensive-guide-tips-and-tricks-for-gas-optimization-in-solidity-5380db734404) of the rule initiciative (see Calldata vs Memory) +- [source 2](https://www.rareskills.io/post/gas-optimization?postId=c9db474a-ff97-4fa3-a51d-fe13ccb8fe3b#viewer-6acr7) of the rule initiciative + +## Examples +This rule does not have examples. + +## Version +This rule is introduced in the latest version. + +## Resources +- [Rule source](https://github.com/protofire/solhint/tree/master/lib/rules/gas-consumption/gas-calldata-parameters.js) +- [Document source](https://github.com/protofire/solhint/tree/master/docs/rules/gas-consumption/gas-calldata-parameters.md) +- [Test cases](https://github.com/protofire/solhint/tree/master/test/rules/gas-consumption/gas-calldata-parameters.js) diff --git a/lib/rules/gas-consumption/gas-calldata-parameters.js b/lib/rules/gas-consumption/gas-calldata-parameters.js new file mode 100644 index 00000000..df1a598b --- /dev/null +++ b/lib/rules/gas-consumption/gas-calldata-parameters.js @@ -0,0 +1,111 @@ +const BaseChecker = require('../base-checker') + +const solidityOperators = [ + '=', + '++', + '--', + '+=', + '-=', + '*=', + '/=', + '%=', + '&=', + '|=', + '^=', + '<<=', + '>>=', +] +const ruleId = 'gas-calldata-parameters' +const meta = { + type: 'gas-consumption', + + docs: { + description: 'Suggest calldata keyword on function arguments when read only', + category: 'Gas Consumption Rules', + notes: [ + { + note: 'Only applies for external functions when receiving arguments with [memory] keyword', + }, + { + note: 'This rule makes a soft check to see if argument is readOnly to make the suggestion. Check it manually before changing it.', + }, + { + note: '[source 1](https://coinsbench.com/comprehensive-guide-tips-and-tricks-for-gas-optimization-in-solidity-5380db734404) of the rule initiciative (see Calldata vs Memory)', + }, + { + note: '[source 2](https://www.rareskills.io/post/gas-optimization?postId=c9db474a-ff97-4fa3-a51d-fe13ccb8fe3b#viewer-6acr7) of the rule initiciative', + }, + ], + }, + + isDefault: false, + recommended: false, + defaultSetup: 'warn', + + schema: null, +} + +class GasCalldataParameters extends BaseChecker { + constructor(reporter) { + super(reporter, ruleId, meta) + } + + // checkIfReadOnly(statements, functionArguments) { + checkIfReadOnly(node, index) { + let isUpdated = false + let i = 0 + const statements = node.body.statements + + while (i < statements.length) { + if (statements[i].type === 'ExpressionStatement') { + // variable manipulation + if ( + statements[i].expression.type === 'BinaryOperation' && + solidityOperators.includes(statements[i].expression.operator) + ) { + // array + if (statements[i].expression.left.base && statements[i].expression.left.base.name) { + isUpdated = statements[i].expression.left.base.name === node.parameters[index].name + } // regular expression + else if (statements[i].expression.left.name) { + isUpdated = statements[i].expression.left.name === node.parameters[index].name + } // struct + else if (statements[i].expression.left.type === 'MemberAccess') { + isUpdated = + statements[i].expression.left.expression.name === node.parameters[index].name + } + } + } + // if is updated stop looping + if (isUpdated) i = statements.length + // if not, keep looping statements + else i++ + } + + return isUpdated + } + + FunctionDefinition(node) { + const qtyArguments = node.parameters.length + let isUpdated = false + + // check for external functions the memory keyword + if (node.visibility === 'external' && qtyArguments > 0) { + for (let i = 0; i < qtyArguments; i++) { + if (node.parameters[i].storageLocation === 'memory') { + isUpdated = this.checkIfReadOnly(node, i) + if (!isUpdated) this.reportError(node, node.parameters[i].name) + } + } + } + } + + reportError(node, parameterName) { + this.error( + node, + `GC: [${parameterName}] argument on Function [${node.name}] could be [calldata] if it's not being updated` + ) + } +} + +module.exports = GasCalldataParameters diff --git a/lib/rules/gas-consumption/index.js b/lib/rules/gas-consumption/index.js index 89ea5c7b..68dc40f7 100644 --- a/lib/rules/gas-consumption/index.js +++ b/lib/rules/gas-consumption/index.js @@ -1,6 +1,7 @@ const GasMultitoken1155 = require('./gas-multitoken1155') const GasSmallStrings = require('./gas-small-strings') const GasIndexedEvents = require('./gas-indexed-events') +const GasCalldataParameters = require('./gas-calldata-parameters') // module.exports = function checkers(reporter, config, tokens) { module.exports = function checkers(reporter, config) { @@ -8,5 +9,7 @@ module.exports = function checkers(reporter, config) { new GasMultitoken1155(reporter, config), new GasSmallStrings(reporter, config), new GasIndexedEvents(reporter, config), + new GasIndexedEvents(reporter, config), + new GasCalldataParameters(reporter, config), ] } diff --git a/test/rules/gas-consumption/gas-calldata-parameters.js b/test/rules/gas-consumption/gas-calldata-parameters.js new file mode 100644 index 00000000..9977b691 --- /dev/null +++ b/test/rules/gas-consumption/gas-calldata-parameters.js @@ -0,0 +1,113 @@ +const assert = require('assert') +const linter = require('../../../lib/index') +const { contractWith } = require('../../common/contract-builder') + +const replaceErrorMsg = (parameterName, functionName) => { + const errMsg = `GC: [${parameterName}] argument on Function [${functionName}] could be [calldata] if it's not being updated` + return errMsg +} + +describe('Linter - gas-calldata-parameters', () => { + it('should raise error on strVar', () => { + const code = contractWith(` + function function1( + uint256[] memory arrayUint, + bool active, + string memory strVar + ) external pure { + active = true; + arrayUint[0] = 1; + } + `) + + const report = linter.processStr(code, { + rules: { 'gas-calldata-parameters': 'error' }, + }) + + assert.equal(report.errorCount, 1) + assert.equal(report.messages[0].message, replaceErrorMsg('strVar', 'function1')) + }) + + it('should raise error on strVar and arrayUint', () => { + const code = contractWith(` + function function1( + uint256[] memory arrayUint, + bool active, + string memory strVar + ) external pure { + active = true; + } + `) + + const report = linter.processStr(code, { + rules: { 'gas-calldata-parameters': 'error' }, + }) + + assert.equal(report.errorCount, 2) + assert.equal(report.messages[0].message, replaceErrorMsg('arrayUint', 'function1')) + assert.equal(report.messages[1].message, replaceErrorMsg('strVar', 'function1')) + }) + + it('should raise error on arrayUint', () => { + const code = contractWith(` + function function1( + uint256[] memory arrayUint, + bool active, + CustomStruct memory customStruct + ) external pure { + customStruct.field1 = true; + } + `) + + const report = linter.processStr(code, { + rules: { 'gas-calldata-parameters': 'error' }, + }) + + assert.equal(report.errorCount, 1) + assert.equal(report.messages[0].message, replaceErrorMsg('arrayUint', 'function1')) + }) + + it('should raise error on arrayUint, customStruct and strVar', () => { + const code = contractWith(` + function functionTest1( + uint256[] memory arrayUint, + bool active, + CustomStruct memory customStruct, + string memory strVar + ) external pure { + active = true; + string strVar2 = strVar; + active = customStruct.active; + } + `) + + const report = linter.processStr(code, { + rules: { 'gas-calldata-parameters': 'error' }, + }) + + assert.equal(report.errorCount, 3) + assert.equal(report.messages[0].message, replaceErrorMsg('arrayUint', 'functionTest1')) + assert.equal(report.messages[1].message, replaceErrorMsg('customStruct', 'functionTest1')) + assert.equal(report.messages[2].message, replaceErrorMsg('strVar', 'functionTest1')) + }) + + it('should NOT raise error', () => { + const code = contractWith(` + function function1( + uint256[] memory arrayUint, + bool active, + string memory strVar + ) external pure { + active = true; + arrayUint[0] = 1; + strVar = ''; + } + `) + + const report = linter.processStr(code, { + rules: { 'gas-calldata-parameters': 'error' }, + }) + + assert.equal(report.errorCount, 0) + }) +}) From 548af69c5af2620f74b2d46c89daa8d47c014bac Mon Sep 17 00:00:00 2001 From: dbale-altoros Date: Fri, 19 Jan 2024 11:35:55 -0300 Subject: [PATCH 2/2] fix: added logic for other asignations --- .../gas-calldata-parameters.js | 36 ++++++++++------ lib/rules/gas-consumption/index.js | 1 - .../gas-calldata-parameters.js | 42 +++++++++++++++++++ 3 files changed, 65 insertions(+), 14 deletions(-) diff --git a/lib/rules/gas-consumption/gas-calldata-parameters.js b/lib/rules/gas-consumption/gas-calldata-parameters.js index df1a598b..c53aa3d4 100644 --- a/lib/rules/gas-consumption/gas-calldata-parameters.js +++ b/lib/rules/gas-consumption/gas-calldata-parameters.js @@ -59,20 +59,30 @@ class GasCalldataParameters extends BaseChecker { while (i < statements.length) { if (statements[i].type === 'ExpressionStatement') { // variable manipulation - if ( - statements[i].expression.type === 'BinaryOperation' && - solidityOperators.includes(statements[i].expression.operator) - ) { - // array - if (statements[i].expression.left.base && statements[i].expression.left.base.name) { - isUpdated = statements[i].expression.left.base.name === node.parameters[index].name - } // regular expression - else if (statements[i].expression.left.name) { - isUpdated = statements[i].expression.left.name === node.parameters[index].name - } // struct - else if (statements[i].expression.left.type === 'MemberAccess') { + if (solidityOperators.includes(statements[i].expression.operator)) { + // binary operation + if (statements[i].expression.type === 'BinaryOperation') { + // array + if (statements[i].expression.left.base && statements[i].expression.left.base.name) { + isUpdated = statements[i].expression.left.base.name === node.parameters[index].name + } + // regular expression + else if (statements[i].expression.left.name) { + isUpdated = statements[i].expression.left.name === node.parameters[index].name + } + // struct + else if (statements[i].expression.left.type === 'MemberAccess') { + isUpdated = + statements[i].expression.left.expression.name === node.parameters[index].name + } + } + // unary operations + else if (statements[i].expression.type === 'UnaryOperation') { isUpdated = - statements[i].expression.left.expression.name === node.parameters[index].name + // regular unary expression + statements[i].expression.subExpression.name === node.parameters[index].name || + // on array expression + statements[i].expression.subExpression.base.name === node.parameters[index].name } } } diff --git a/lib/rules/gas-consumption/index.js b/lib/rules/gas-consumption/index.js index 68dc40f7..41fc27ca 100644 --- a/lib/rules/gas-consumption/index.js +++ b/lib/rules/gas-consumption/index.js @@ -9,7 +9,6 @@ module.exports = function checkers(reporter, config) { new GasMultitoken1155(reporter, config), new GasSmallStrings(reporter, config), new GasIndexedEvents(reporter, config), - new GasIndexedEvents(reporter, config), new GasCalldataParameters(reporter, config), ] } diff --git a/test/rules/gas-consumption/gas-calldata-parameters.js b/test/rules/gas-consumption/gas-calldata-parameters.js index 9977b691..b8cfd07d 100644 --- a/test/rules/gas-consumption/gas-calldata-parameters.js +++ b/test/rules/gas-consumption/gas-calldata-parameters.js @@ -91,6 +91,48 @@ describe('Linter - gas-calldata-parameters', () => { assert.equal(report.messages[2].message, replaceErrorMsg('strVar', 'functionTest1')) }) + it('should raise error on customStruct and strVar', () => { + const code = contractWith(` + function functionTest1( + uint256[] memory arrayUint, + bool active, + CustomStruct memory customStruct, + string memory strVar + ) external pure { + arrayUint[i]++; + } + `) + + const report = linter.processStr(code, { + rules: { 'gas-calldata-parameters': 'error' }, + }) + + assert.equal(report.errorCount, 2) + assert.equal(report.messages[0].message, replaceErrorMsg('customStruct', 'functionTest1')) + assert.equal(report.messages[1].message, replaceErrorMsg('strVar', 'functionTest1')) + }) + + it('should raise error on arrayUint', () => { + const code = contractWith(` + function functionTest1( + uint256[] memory arrayUint, + bool active, + CustomStruct memory customStruct, + string memory strVar + ) external pure { + customStruct.field1 = "something"; + strVar <<= active; + } + `) + + const report = linter.processStr(code, { + rules: { 'gas-calldata-parameters': 'error' }, + }) + + assert.equal(report.errorCount, 1) + assert.equal(report.messages[0].message, replaceErrorMsg('arrayUint', 'functionTest1')) + }) + it('should NOT raise error', () => { const code = contractWith(` function function1(