Skip to content

Conversation

@k06a
Copy link
Member

@k06a k06a commented Jul 23, 2025

Static Code Analysis (readability, compactness):

  1. Added Transient.sol:

    • Core library for transient storage operations (requires Solidity ^0.8.24)
    • Provides transient types:
      • tuint256
      • taddress
      • tbytes32
    • With access methods:
      • .tload(slot) - load value from transient storage
      • .tstore(slot, value) - store value in transient storage
      • .inc() - increment with overflow check
      • .inc(bytes4 exception) - increment with custom exception
      • .dec() - decrement with underflow check
      • .dec(bytes4 exception) - decrement with custom exception
      • .unsafeInc() - unchecked increment
      • .unsafeDec() - unchecked decrement
  2. Added TransientArray.sol:

    • Dynamic arrays using transient storage (requires Solidity ^0.8.24)
    • Three array types:
      • TransientArray.Uint256
      • TransientArray.Address
      • TransientArray.Bytes32
    • Methods for each type:
      • .length() - get array length
      • .at(index) - get element with bounds checking
      • .unsafeAt(index) - get element without bounds checking
      • .get() - return entire array as memory array
      • .set(index, value) - set element at index
      • .push(value) - append element, returns new length
      • .pop() - remove and return last element
  3. Added TransientSet.sol:

    • Set data structure using transient storage (requires Solidity ^0.8.24)
    • Three set types with O(1) operations:
      • TransientSet.Uint256
      • TransientSet.Address
      • TransientSet.Bytes32
    • Methods for each type:
      • .length() - get set size
      • .at(index) - get element at index
      • .unsafeAt(index) - get element without bounds checking
      • .contains(item) - check if item exists in set
      • .get() - return entire set as memory array
      • .add(item) - add item to set (returns false if already exists)
      • .remove(item) - remove item from set (returns false if not found)
    • Uses inverted index storage pattern (~index) for efficient lookups
  4. Added TransientLock.sol:

    • Primitive for transient lock management (requires Solidity ^0.8.24)
    • Provides TransientLock struct and TransientLockLib library
    • Core building block for reentrancy protection
  5. Added ReentrancyGuard.sol:

    • Abstract contract for reentrancy protection using transient storage
    • Built on top of TransientLock
    • Provides modifiers for protecting functions from reentrancy
  6. Added MsgSender.sol:

    • Sophisticated message sender tracking system
    • Maintains per-caller, per-selector transient stacks
    • Structure: mapping(address caller => mapping(bytes4 selector => TransientArray.Address))
    • Methods:
      • _msgSender(caller, selector, index) - get sender at specific index
      • _msgSenderLength(caller, selector) - get stack length for selector
      • _msgSenderPush(caller, selector, newSender) - push sender to stack
      • _msgSenderPop(caller, selector, expected) - pop with validation
      • _msgSender() - returns top of stack or falls back to Context
  7. Modified BySig.sol:

    • Replaced Context inheritance with MsgSender inheritance
    • Removed private _msgSenders TransientArray.Address field
    • Delegated to MsgSender with per-selector granularity
    • Uses _msgSenderPush/Pop with function selector tracking
  8. Modified TokenWithBySig.sol:

    • Updated _msgSender() override to use MsgSender instead of BySig

Dynamic Code Analysis (external APIs, interaction flows):

  • Complete transient storage infrastructure for Solidity 0.8.24+
  • Per-selector message sender tracking enables:
    • Independent context for each function
    • Prevention of cross-function pollution
    • Support for concurrent meta-transactions
  • Set operations maintain O(1) complexity for add/remove/contains
  • Reentrancy protection using transient locks instead of storage
  • Automatic cleanup of transient data after transaction completion

Efficiency (gas costs, computational complexity, memory requirements):

  • Zero-cost abstraction over TSTORE/TLOAD opcodes
  • Transient storage is cheaper than regular storage for temporary data
  • O(1) set operations through inverted index pattern
  • Automatic cleanup reduces gas refunds complexity
  • Per-selector tracking adds minimal overhead with significant security benefits

Opinion, trade-offs and other thoughts (optional):

  • Comprehensive transient storage library suite covering primitives to complex data structures
  • Per-selector granularity in MsgSender is a sophisticated architectural choice for better isolation
  • TransientSet's inverted index pattern (~index) is clever for maintaining O(1) operations
  • Transient storage is ideal for reentrancy guards and temporary state tracking
  • Requires Solidity 0.8.24+ which may limit adoption until ecosystems upgrade

@codecov
Copy link

codecov bot commented Jul 23, 2025

Codecov Report

❌ Patch coverage is 18.14346% with 194 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.26%. Comparing base (94611cc) to head (49229e2).
⚠️ Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
contracts/libraries/TransientSet.sol 0.00% 81 Missing ⚠️
contracts/libraries/TransientArray.sol 19.75% 65 Missing ⚠️
contracts/libraries/Transient.sol 28.57% 20 Missing ⚠️
contracts/libraries/ReentrancyGuard.sol 0.00% 16 Missing ⚠️
contracts/libraries/TransientLock.sol 0.00% 8 Missing ⚠️
contracts/libraries/MsgSender.sol 75.00% 4 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (94611cc) and HEAD (49229e2). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (94611cc) HEAD (49229e2)
2 1
Additional details and impacted files
@@             Coverage Diff              @@
##            master     #197       +/-   ##
============================================
- Coverage   100.00%   66.26%   -33.74%     
============================================
  Files           17       23        +6     
  Lines          346      575      +229     
  Branches        65       89       +24     
============================================
+ Hits           346      381       +35     
- Misses           0      194      +194     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@k06a k06a force-pushed the feature/transient branch 2 times, most recently from 3852dee to c444061 Compare July 26, 2025 00:27
@k06a k06a force-pushed the feature/transient branch from c444061 to b54387f Compare July 26, 2025 00:28
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces transient storage capabilities by adding libraries for structured transient data types and reentrancy protection, and refactors the BySig mixin to use these new abstractions for better separation of concerns.

  • Adds transient storage abstractions (Transient.sol, TransientArray.sol) that wrap TSTORE/TLOAD operations
  • Introduces reentrancy protection using transient locks (TransientLock.sol, ReentrancyGuard.sol)
  • Refactors BySig.sol to use TransientArray and extract MsgSender functionality into a separate library

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
contracts/libraries/Transient.sol Core transient storage types (tuint256, taddress, tbytes32) with basic operations
contracts/libraries/TransientArray.sol Transient dynamic arrays for uint256, address, and bytes32 types
contracts/libraries/TransientLock.sol Transient lock primitive for synchronization
contracts/libraries/ReentrancyGuard.sol Abstract contract providing reentrancy protection using transient locks
contracts/mixins/BySig.sol Refactored to use TransientArray and MsgSender library
contracts/tests/mocks/TokenWithBySig.sol Updated to use MsgSender instead of BySig for _msgSender override

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +66 to +70
uint256 len = self._length.tload();
array = new uint256[](len);
for (uint256 i = 0; i < len; i++) {
array[i] = self._items[i].tload();
}
Copy link

Copilot AI Sep 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The get() function performs multiple TLOAD operations in a loop, which could be gas-intensive for large arrays. Consider adding documentation about the gas implications or providing a warning about using this function with large arrays.

Copilot uses AI. Check for mistakes.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor

@k06a k06a force-pushed the feature/transient branch from 1317a09 to 05d9d32 Compare September 10, 2025 11:56
@k06a k06a force-pushed the feature/transient branch from 05d9d32 to 790d70c Compare September 10, 2025 12:08
@k06a k06a changed the title Add Transient.sol and TransientArray.sol Add transient primitives, array, set and some other stuff Sep 10, 2025
@k06a k06a requested a review from Copilot September 11, 2025 05:56
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 9 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@k06a k06a requested a review from Copilot September 12, 2025 17:14
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@k06a k06a requested a review from Copilot September 12, 2025 19:36
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.


struct Bytes32 {
TransientArray.Bytes32 _items;
mapping(bytes32 => tuint256) _lookup; // stored as index, similar to +1 but unchecked math
Copy link

Copilot AI Sep 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment 'similar to +1 but unchecked math' is unclear and doesn't accurately describe the lookup mechanism. The lookup stores 1-based indices (index + 1) to distinguish between unset (0) and first element (1). Consider clarifying this implementation detail.

Suggested change
mapping(bytes32 => tuint256) _lookup; // stored as index, similar to +1 but unchecked math
mapping(bytes32 => tuint256) _lookup; // Stores 1-based indices (index + 1) to distinguish between unset (0) and first element (1)

Copilot uses AI. Check for mistakes.
Comment on lines +17 to +22
/// require(myArray._length() == 1, "Array should not be empty after push");
/// require(myArray.at(0) == value, "Value at index 0 does not match pushed value");
///
/// uint256 poppedValue = myArray.pop();
/// require(poppedValue == value, "Popped value does not match pushed value");
/// require(myArray._length() == 0, "Array should be empty after pop");
Copy link

Copilot AI Sep 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation example uses _length() which doesn't match the actual function name length(). This could confuse developers trying to use the library.

Suggested change
/// require(myArray._length() == 1, "Array should not be empty after push");
/// require(myArray.at(0) == value, "Value at index 0 does not match pushed value");
///
/// uint256 poppedValue = myArray.pop();
/// require(poppedValue == value, "Popped value does not match pushed value");
/// require(myArray._length() == 0, "Array should be empty after pop");
/// require(myArray.length() == 1, "Array should not be empty after push");
/// require(myArray.at(0) == value, "Value at index 0 does not match pushed value");
///
/// uint256 poppedValue = myArray.pop();
/// require(poppedValue == value, "Popped value does not match pushed value");
/// require(myArray.length() == 0, "Array should be empty after pop");

Copilot uses AI. Check for mistakes.
@@ -0,0 +1,45 @@
// SPDX-License-Identifier: MIT

pragma solidity ^0.8.0;
Copy link

Copilot AI Sep 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pragma version should be ^0.8.24 to match the other transient storage files, since this contract uses TransientArray which requires the TSTORE/TLOAD opcodes.

Suggested change
pragma solidity ^0.8.0;
pragma solidity ^0.8.24;

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant