Repository: protofire/solhint
Pull request #546 (2024-01-24 19:27:03)

GC: Struct packing


Patch:
diff --git a/conf/rulesets/solhint-all.js b/conf/rulesets/solhint-all.js
index d2fc240..852ce67 100644
--- a/conf/rulesets/solhint-all.js
+++ b/conf/rulesets/solhint-all.js
@@ -30,6 +30,7 @@ module.exports = Object.freeze({
     'gas-indexed-events': 'warn',
     'gas-multitoken1155': 'warn',
     'gas-small-strings': 'warn',
+    'gas-struct-packing': 'warn',
     'comprehensive-interface': 'warn',
     quotes: ['error', 'double'],
     'const-name-snakecase': 'warn',
diff --git a/docs/rules.md b/docs/rules.md
index bb220f3..356d1d7 100644
--- a/docs/rules.md
+++ b/docs/rules.md
@@ -34,6 +34,7 @@ title:       "Rule Index of Solhint"
 | [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                                |             |            |
+| [gas-struct-packing](./rules/gas-consumption/gas-struct-packing.md)           | Suggest to re-arrange struct packing order when it is inefficient |             |            |
         
 
 ## Miscellaneous
diff --git a/docs/rules/gas-consumption/gas-struct-packing.md b/docs/rules/gas-consumption/gas-struct-packing.md
new file mode 100644
index 0000000..ca2eb40
--- /dev/null
+++ b/docs/rules/gas-consumption/gas-struct-packing.md
@@ -0,0 +1,40 @@
+---
+warning:     "This is a dynamically generated file. Do not edit manually."
+layout:      "default"
+title:       "gas-struct-packing | Solhint"
+---
+
+# gas-struct-packing
+![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 to re-arrange struct packing order when it is inefficient
+
+## 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-struct-packing": "warn"
+  }
+}
+```
+
+### Notes
+- This rule assumes all UserDefinedTypeName take a new slot. (beware of Enums inside Structs) 
+- [source 1](https://coinsbench.com/comprehensive-guide-tips-and-tricks-for-gas-optimization-in-solidity-5380db734404) of the rule initiative (Variable Packing)
+- [source 2](https://www.rareskills.io/post/gas-optimization?postId=c9db474a-ff97-4fa3-a51d-fe13ccb8fe3b#viewer-f8m1r) of the rule initiative
+
+## 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-struct-packing.js)
+- [Document source](https://github.com/protofire/solhint/tree/master/docs/rules/gas-consumption/gas-struct-packing.md)
+- [Test cases](https://github.com/protofire/solhint/tree/master/test/rules/gas-consumption/gas-struct-packing.js)
diff --git a/lib/rules/gas-consumption/gas-increment-by-one.js b/lib/rules/gas-consumption/gas-increment-by-one.js
index d134046..74fc8a5 100644
--- a/lib/rules/gas-consumption/gas-increment-by-one.js
+++ b/lib/rules/gas-consumption/gas-increment-by-one.js
@@ -1,10 +1,6 @@
 /* eslint-disable */
-const { isArray } = require('lodash')
 const BaseChecker = require('../base-checker')
 
-const operators = ['+', '-', '++', '--']
-const binaryOperators = ['+', '-', '+=', '-=']
-
 const ruleId = 'gas-increment-by-one'
 const meta = {
   type: 'gas-consumption',
diff --git a/lib/rules/gas-consumption/gas-struct-packing.js b/lib/rules/gas-consumption/gas-struct-packing.js
new file mode 100644
index 0000000..f8811d5
--- /dev/null
+++ b/lib/rules/gas-consumption/gas-struct-packing.js
@@ -0,0 +1,184 @@
+/* eslint-disable */
+const BaseChecker = require('../base-checker')
+
+const ruleId = 'gas-struct-packing'
+const meta = {
+  type: 'gas-consumption',
+
+  docs: {
+    description: 'Suggest to re-arrange struct packing order when it is inefficient',
+    category: 'Gas Consumption Rules',
+    notes: [
+      {
+        note: 'This rule assumes all UserDefinedTypeName take a new slot. (beware of Enums inside Structs) ',
+      },
+      {
+        note: '[source 1](https://coinsbench.com/comprehensive-guide-tips-and-tricks-for-gas-optimization-in-solidity-5380db734404) of the rule initiative (Variable Packing)',
+      },
+      {
+        note: '[source 2](https://www.rareskills.io/post/gas-optimization?postId=c9db474a-ff97-4fa3-a51d-fe13ccb8fe3b#viewer-f8m1r) of the rule initiative',
+      },
+    ],
+  },
+
+  isDefault: false,
+  recommended: false,
+  defaultSetup: 'warn',
+
+  schema: null,
+}
+
+class GasStructPacking extends BaseChecker {
+  constructor(reporter) {
+    super(reporter, ruleId, meta)
+  }
+
+  StructDefinition(node) {
+    const reportError = this.isInefficientlyPacked(node)
+    if (reportError) {
+      this.reportError(node)
+    }
+  }
+
+  isInefficientlyPacked(structNode) {
+    if (!structNode || !Array.isArray(structNode.members) || structNode.members.length <= 1) {
+    // if (!structNode || !Array.isArray(structNode.members)) {
+      return false // Early return for structs with 1 or no members
+    }
+
+    let members = structNode.members.map((member) => ({
+      name: member.name,
+      size: this.getVariableSize(member.typeName),
+      type: member.typeName.type,
+    }))
+
+    const canBeImproved = this.analyzePacking(members)
+    return canBeImproved
+  }
+
+  // Function to calculate the optimal slots needed for given members
+  calculateOptimalSlots(arr) {
+    let totalSize = 0
+    arr.forEach(({ size }) => {
+      totalSize += size
+    })
+    return Math.ceil(totalSize / 32)
+  }
+
+  calculateCurrentSlotsUsed(members) {
+    let slotsUsed = 0;
+    let currentSlotSpace = 0;
+  
+    members.forEach(member => {
+      if (member.size === 32) {
+        if (currentSlotSpace > 0) {
+          slotsUsed += 1; // Finish the current slot if it was partially filled
+          currentSlotSpace = 0;
+        }
+        slotsUsed += 1; // This member occupies a full slot
+      } else {
+        if (currentSlotSpace + member.size > 32) {
+          slotsUsed += 1; // Move to the next slot if adding this member exceeds the current slot
+          currentSlotSpace = member.size; // Start filling the next slot
+        } else {
+          currentSlotSpace += member.size; // Add to the current slot space
+        }
+      }
+    });
+  
+    // If there's any space used in the current slot after looping, it means another slot is partially filled
+    if (currentSlotSpace > 0) {
+      slotsUsed += 1;
+    }
+  
+    return slotsUsed;
+  }
+
+  
+  analyzePacking(members) {
+    // Separate members into large and small for analysis
+    // const largeMembers = members.filter((member) => member.size === 32)
+    const smallMembers = members.filter((member) => member.size < 32)
+
+    // Sort small members by size, descending, to optimize packing
+    smallMembers.sort((a, b) => b.size - a.size)
+
+    // Initial slots count: one slot per large member
+    let currentSlots = this.calculateCurrentSlotsUsed(members)
+
+    // Track remaining space in the current slot
+    let remainingSpace = 32
+    smallMembers.forEach((member) => {
+      if (member.size <= remainingSpace) {
+        // If the member fits in the current slot, subtract its size from the remaining space
+        remainingSpace -= member.size
+      } else {
+        // If not, start a new slot and adjust remaining space
+        remainingSpace = 32 - member.size
+      }
+    })
+
+    // Calculate optimal slots needed if perfectly packed
+    const optimalSlots = this.calculateOptimalSlots(members)
+
+    // console.log(`\n\nCurrent slot usage: ${currentSlots}`)
+    // console.log(`Optimal (minimum possible) slot usage: ${optimalSlots}`)
+
+    return currentSlots > optimalSlots
+  }
+
+  getVariableSize(typeNode) {
+    if (!typeNode) {
+      return 32
+    }
+
+    switch (typeNode.type) {
+      case 'ElementaryTypeName':
+        return this.getSizeForElementaryType(typeNode.name)
+      case 'UserDefinedTypeName':
+        // this can be improved for enums
+        return 32
+      case 'ArrayTypeName':
+        if (typeNode.length) {
+          // if array is fixed, get the length * var size
+          const varSize = this.getSizeForElementaryType(typeNode.baseTypeName.name)
+          const size = parseInt(typeNode.length.number) * varSize
+          return size > 32 ? 32 : size
+        }
+        return 32 // Dynamic arrays occupy a full slot
+
+      default:
+        return 32
+    }
+  }
+
+  getSizeForElementaryType(typeName) {
+    switch (typeName) {
+      case 'address':
+        return 20
+      case 'bool':
+        return 1
+      case 'string':
+      case 'bytes':
+        return 32
+      default:
+        return typeName.includes('uint') || typeName.includes('int')
+          ? this.getSizeForIntType(typeName)
+          : 32
+    }
+  }
+
+  getSizeForIntType(typeName) {
+    const bits = parseInt(typeName.replace(/\D/g, ''), 10)
+    return Math.ceil(bits / 8)
+  }
+
+  reportError(node) {
+    this.error(
+      node,
+      `GC: For [ ${node.name} ] struct, packing seems inefficient. Try rearranging to achieve 32bytes slots`
+    )
+  }
+}
+
+module.exports = GasStructPacking
diff --git a/lib/rules/gas-consumption/index.js b/lib/rules/gas-consumption/index.js
index 4c7a5ee..e030785 100644
--- a/lib/rules/gas-consumption/index.js
+++ b/lib/rules/gas-consumption/index.js
@@ -2,15 +2,16 @@ const GasMultitoken1155 = require('./gas-multitoken1155')
 const GasSmallStrings = require('./gas-small-strings')
 const GasIndexedEvents = require('./gas-indexed-events')
 const GasCalldataParameters = require('./gas-calldata-parameters')
-const IncrementByOne = require('./gas-increment-by-one')
+const GasIncrementByOne = require('./gas-increment-by-one')
+const GasStructPacking = require('./gas-struct-packing')
 
-// module.exports = function checkers(reporter, config, tokens) {
 module.exports = function checkers(reporter, config) {
   return [
     new GasMultitoken1155(reporter, config),
     new GasSmallStrings(reporter, config),
     new GasIndexedEvents(reporter, config),
     new GasCalldataParameters(reporter, config),
-    new IncrementByOne(reporter, config),
+    new GasIncrementByOne(reporter, config),
+    new GasStructPacking(reporter, config),
   ]
 }


Test patch:
diff --git a/test/fixtures/gas-consumption/gas-struct-packing-data.js b/test/fixtures/gas-consumption/gas-struct-packing-data.js
new file mode 100644
index 0000000..ae70464
--- /dev/null
+++ b/test/fixtures/gas-consumption/gas-struct-packing-data.js
@@ -0,0 +1,148 @@
+const { contractWith, multiLine } = require('../../common/contract-builder')
+
+const contractStructsInefficient = [
+  {
+    name: 'Inefficient1',
+    code: contractWith(
+      multiLine(
+        // Mixed Small and Large Types
+        'struct Inefficient1 {',
+        '    uint8 smallValue1; // 1 byte',
+        '    uint256 largeValue; // 32 bytes, starts new slot due to size',
+        '    uint8 smallValue2; // 1 byte, starts new slot, inefficient',
+        '}'
+      )
+    ),
+  },
+  {
+    name: 'Inefficient2',
+    code: contractWith(
+      multiLine(
+        // // Small Types Followed by Dynamic Type
+        'struct Inefficient2 {',
+        '    uint16 smallValue; // 2 bytes',
+        '    address addr; // 20 bytes',
+        '    string dynamicData; // dynamically-sized, starts new slot',
+        '    uint8 extraSmall; // 1 byte, starts new slot, inefficient',
+        '}'
+      )
+    ),
+  },
+  {
+    name: 'Inefficient3',
+    code: contractWith(
+      multiLine(
+        // // Unaligned Array with Small Types
+        'struct Inefficient3 {',
+        '    uint32 smallValue; // 4 bytes, starts new slot, inefficient',
+        '   uint32[] dynamicArray; // dynamically-sized, starts new slot',
+        '   uint32[2] nonDynamicArray8Bytes; // 8 bytes non dynamically-sized',
+        '   uint32[3] nonDynamicArray12Bytes; // 12 bytes non dynamically-sized ',
+        '   bool flag; // 1 byte, starts new slot, inefficient',
+        '}'
+      )
+    ),
+  },
+  {
+    name: 'Inefficient4',
+    code: contractWith(
+      multiLine(
+        // smallValue4b can be on top
+        'struct Inefficient4 {',
+        '   uint16 smallValue2b; // 2 bytes',
+        '   uint16[2] fixedArray; // 4 bytes',
+        '   uint32[] dynamicArray; // 32 dynamically-sized, starts new slot',
+        '   uint128 smallValue16b; // 16 bytes',
+        '   uint128 smallValue16b2; // 16 bytes',
+        '   uint32 smallValue4b; // 4 bytes',
+        '   uint256 largeValue; // 32 bytes',
+        '   uint256[20] veryLargeArray; // 640 bytes',
+        '   mapping(address => uint256) whatever;',
+        '}'
+      )
+    ),
+  },
+]
+
+const contractStructsEfficient = [
+  {
+    name: 'Efficient1',
+    code: contractWith(
+      multiLine(
+        // // Packing Small Types Together
+        'struct Efficient1 {',
+        '   uint8 smallValue1; // 1 byte',
+        '   uint8 smallValue2; // 1 byte, same slot',
+        '   uint16 smallValue3; // 2 bytes, same slot',
+        '   uint8 smallValue4; // 1 byte, same slot',
+        '   uint8 smallValue5; // 1 byte, same slot',
+        '   uint8 smallValue6; // 1 byte, same slot',
+        '   uint8 smallValue7; // 1 byte, same slot',
+        '   address addr; // 20 bytes, starts new slot',
+        '}'
+      )
+    ),
+  },
+  {
+    name: 'Efficient2',
+    code: contractWith(
+      multiLine(
+        // Aligning Fixed-Size Arrays
+        'struct Efficient2 {',
+        '   uint256[20] veryLargeArray1; // 640 bytes starts new slot',
+        '   uint16[2] fixedArray; // 4 bytes total, same slot',
+        '   uint16 smallValue1; // 2 bytes, same slot',
+        '   uint32 smallValue2; // 4 bytes, same slot',
+        '   uint256 largeValue; // 32 bytes, starts new slot',
+        '   uint256[20] veryLargeArray2; // 640 bytes starts new slot',
+        '}'
+      )
+    ),
+  },
+  {
+    name: 'Efficient3',
+    code: contractWith(
+      multiLine(
+        // // Large Types Followed by Small Types
+        'struct Efficient3 {',
+        '   uint256 largeValue1; // 32 bytes, full slot',
+        '   uint256 largeValue2; // 32 bytes, new slot',
+        '   uint8 smallValue1; // 1 byte, new slot',
+        '   bool flag; // 1 byte, same slot',
+        '   address addr; // 20 bytes, same slot',
+        '}'
+      )
+    ),
+  },
+  {
+    name: 'Efficient4',
+    code: contractWith(
+      multiLine(
+        'struct Efficient4 {',
+        '   uint32[] dynamicArray; // dynamically-sized, starts new slot',
+        '   uint32 smallValue; // 4 bytes, starts new slot, inefficient',
+        '   bool flag; // 1 byte, starts new slot, inefficient',
+        '}'
+      )
+    ),
+  },
+  {
+    name: 'Efficient5',
+    code: contractWith(
+      multiLine(
+        '   struct Efficient5 {',
+        '   uint16 smallValue2b; // 2 bytes',
+        '   uint16[2] fixedArray; // 4 bytes',
+        '   uint128 smallValue16b; // 16 bytes',
+        '   uint128 smallValue16b2; // 16 bytes',
+        '   uint32 smallValue4b; // 4 bytes',
+        '   uint256 largeValue; // 32 bytes',
+        '   uint256[20] veryLargeArray; // 640 bytes',
+        '   mapping(address => uint256) whatever;',
+        '}'
+      )
+    ),
+  },
+]
+
+module.exports = { contractStructsInefficient, contractStructsEfficient }
diff --git a/test/rules/gas-consumption/gas-struct-packing.js b/test/rules/gas-consumption/gas-struct-packing.js
new file mode 100644
index 0000000..975119e
--- /dev/null
+++ b/test/rules/gas-consumption/gas-struct-packing.js
@@ -0,0 +1,33 @@
+const assert = require('assert')
+const linter = require('../../../lib/index')
+const TEST_CASES = require('../../fixtures/gas-consumption/gas-struct-packing-data')
+
+const replaceErrorMsg = (variableName) => {
+  const errMsg = `GC: For [ ${variableName} ] struct, packing seems inefficient. Try rearranging to achieve 32bytes slots`
+  return errMsg
+}
+
+describe('Linter - gas-struct-packingone', () => {
+  for (const contract of TEST_CASES.contractStructsInefficient) {
+    it(`should raise error for ${contract.name}`, () => {
+      const code = contract.code
+      const report = linter.processStr(code, {
+        rules: { 'gas-struct-packing': 'error' },
+      })
+
+      assert.equal(report.errorCount, 1)
+      assert.equal(report.messages[0].message, replaceErrorMsg(contract.name))
+    })
+  }
+
+  for (const contract of TEST_CASES.contractStructsEfficient) {
+    it(`should NOT raise error for ${contract.name}`, () => {
+      const code = contract.code
+      const report = linter.processStr(code, {
+        rules: { 'gas-struct-packing': 'error' },
+      })
+
+      assert.equal(report.errorCount, 0)
+    })
+  }
+})