diff --git a/docs/rules/gas-consumption/gas-struct-packing.md b/docs/rules/gas-consumption/gas-struct-packing.md index 4a3d333c..ca2eb40a 100644 --- a/docs/rules/gas-consumption/gas-struct-packing.md +++ b/docs/rules/gas-consumption/gas-struct-packing.md @@ -24,7 +24,7 @@ This rule accepts a string option of rule severity. Must be one of "error", "war ``` ### Notes -- This rule +- 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 @@ -38,4 +38,3 @@ This rule is introduced in the latest version. - [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) -s) diff --git a/lib/rules/gas-consumption/gas-struct-packing.js b/lib/rules/gas-consumption/gas-struct-packing.js index 0af20cae..f8811d54 100644 --- a/lib/rules/gas-consumption/gas-struct-packing.js +++ b/lib/rules/gas-consumption/gas-struct-packing.js @@ -10,7 +10,7 @@ const meta = { category: 'Gas Consumption Rules', notes: [ { - note: 'This rule ', + 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)', @@ -34,13 +34,15 @@ class GasStructPacking extends BaseChecker { } StructDefinition(node) { - if (this.isInefficientlyPacked(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 } @@ -48,119 +50,81 @@ class GasStructPacking extends BaseChecker { name: member.name, size: this.getVariableSize(member.typeName), type: member.typeName.type, - sum: this.canBeAdded(member.typeName), })) - console.log('members :>> ', members) - - return -/* - let members = structNode.members.map((member) => ({ - name: member.name, - size: this.getVariableSize(member.typeName), - type: member.typeName.type, - newSlot: this.isNewSlot(member.typeName), - })) - - let currentSlotSize = 0 - let overflow = false - - console.log('members :>> ', members) - - // Iterate over struct members to assess packing efficiency - for (const member of members) { - // Special handling for large values that occupy an entire slot or more - if (member.size == 32) { - // If the current slot has been partially used, this large value starts a new slot - if (currentSlotSize > 0) { - currentSlotSize = 0 // Reset for the next slot - overflow = true // Mark that we've overflowed into a new slot - } - // No need to add size to currentSlotSize since it occupies the whole new slot - continue - } - - if (currentSlotSize + member.size > 32) { - // If adding this member exceeds the slot size, move to the next slot - overflow = true // Mark that we've overflowed - currentSlotSize = member.size // This member starts the new slot - } else { - // Accumulate member size in the current slot - currentSlotSize += member.size - } - } - - // If we've never overflowed, all members are efficiently packed - return overflow - */ + const canBeImproved = this.analyzePacking(members) + return canBeImproved } - /* - isInefficientlyPacked(structNode) { - if (!structNode || !Array.isArray(structNode.members) || structNode.members.length <= 1) { - return false - } - - let members = structNode.members.map((member) => ({ - name: member.name, - size: this.getVariableSize(member.typeName), - type: member.typeName.type, - })) - - console.log('members :>> ', members) + // 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) + } - let currentSlotSize = 0; - let isPotentialInefficiencyDetected = false; + calculateCurrentSlotsUsed(members) { + let slotsUsed = 0; + let currentSlotSpace = 0; - // Iterate over struct members to assess packing efficiency members.forEach(member => { - // If adding this member exceeds the current slot size, it starts in a new slot - if (currentSlotSize + member.size > 32) { - // If there was room left in the slot, it's a potential inefficiency - if (currentSlotSize < 32) { - isPotentialInefficiencyDetected = true; + if (member.size === 32) { + if (currentSlotSpace > 0) { + slotsUsed += 1; // Finish the current slot if it was partially filled + currentSlotSpace = 0; } - currentSlotSize = member.size % 32; + slotsUsed += 1; // This member occupies a full slot } else { - currentSlotSize += member.size; - // Adjust current slot size to modulo 32 for cases where exact fit or overflow - currentSlotSize %= 32; + 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 unused space in the last slot, it might not indicate inefficiency by itself - // unless combined with earlier detected potential inefficiency. - if (currentSlotSize > 0 && currentSlotSize < 32 && isPotentialInefficiencyDetected) { - isPotentialInefficiencyDetected = true; - } else if (currentSlotSize === 0 && !isPotentialInefficiencyDetected) { - // If we perfectly fill slots without earlier inefficiency, it's efficient - isPotentialInefficiencyDetected = false; + // 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 isPotentialInefficiencyDetected; - + return slotsUsed; } - */ - - isSlotFull(member) { - console.log('member :>> ', member) - if (!member) { - return false - } - switch (member.type) { - case 'Mapping': - return true + + 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 + } + }) - case 'ArrayTypeName': - return member.size === 32 + // Calculate optimal slots needed if perfectly packed + const optimalSlots = this.calculateOptimalSlots(members) - case 'ElementaryTypeName': - return member.name.startsWith('string') || member.name.startsWith('bytes') + // console.log(`\n\nCurrent slot usage: ${currentSlots}`) + // console.log(`Optimal (minimum possible) slot usage: ${optimalSlots}`) - default: - return false - } + return currentSlots > optimalSlots } getVariableSize(typeNode) { @@ -172,14 +136,17 @@ class GasStructPacking extends BaseChecker { 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) - return parseInt(typeNode.length.number) * varSize + const size = parseInt(typeNode.length.number) * varSize + return size > 32 ? 32 : size } return 32 // Dynamic arrays occupy a full slot + default: return 32 } @@ -206,30 +173,10 @@ class GasStructPacking extends BaseChecker { return Math.ceil(bits / 8) } - canBeAdded(typeName) { - console.log('typeName :>> ', typeName); - - if (typeName.name.includes('uint') || typeName.name.includes('int')) { - const aa = this.getSizeForIntType(typeName.name) - console.log('aa :>> ', aa); - - return !(this.getSizeForIntType(typeName.name) >= 32) - } - console.log("20"); - - switch (typeName.name) { - case 'address': - case 'bool': - return true - default: - return false - } - } - reportError(node) { this.error( node, - `GC: For [ ${node.name} ] struct, packing seems inefficient. Try rearranging variables to achieve 32bytes slots` + `GC: For [ ${node.name} ] struct, packing seems inefficient. Try rearranging to achieve 32bytes slots` ) } } 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 00000000..ae70464e --- /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 00000000..975119e8 --- /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) + }) + } +})