Skip to content

Conversation

@leeftk
Copy link

@leeftk leeftk commented Jul 18, 2025

Funding Pot Contract Major Refactor

Summary

This PR represents a major refactoring of the Funding Pot contract, focusing on simplification, gas optimization, and removal of unnecessary complexity.

Detailed Changes

1. Function Removals

  • Removed getUserEligibility function (288-344)

    • This was redundant as eligibility is checked during contribution
    • Removed associated interface definition
    • Simplified testing by removing direct eligibility checks
  • Removed overloaded contributeToRoundFor function (582-596)

    • Consolidated into single function with unspent caps parameter
    • All calls now require unspent caps array (empty if not using)

2. Error Handling Consolidation

  • Combined multiple round validation errors into RoundParamsInvalid:
    • RoundStartMustBeInFuture
    • RoundMustHaveEndTimeOrCap
    • RoundEndMustBeAfterStart
    • RoundAlreadyStarted
  • Removed parameters from StartRoundGreaterThanRoundCount error
  • Removed InvalidBatchParameters error
  • Removed UnspentCapsRoundIdsNotStrictlyIncreasing
  • Removed UnspentCapsRoundIdsNotContiguous

3. Accumulation Logic Simplification

  • Removed contiguous round ID requirement (659-739)
    • Deleted checks for strictly increasing round IDs
    • Removed validation for sequential round numbers
  • Simplified unspent cap tracking:
    • Moved cap marking to main accumulation loop
    • Removed separate marking block after accumulation
  • Added early continue conditions for invalid rounds
  • Removed redundant round ID validation

4. Event and Parameter Optimization

  • Simplified AllowlistedAddressesRemoved event
    • Removed all parameters (roundId_, accessCriteriaId_, addressesRemoved_)
    • Updated all event emissions
  • Changed merkleProof_ parameter from calldata to memory in interface
  • Removed redundant local variables:
    • Removed currentTime in favor of direct block.timestamp
    • Removed r1UnusedTotal calculation
    • Removed r1PersonalCap duplicate

5. Security Improvements

  • Removed zero address check in _checkNftOwnership (1050-1053)
    • Check was redundant due to upstream validations
    • Improves gas efficiency
  • Simplified batch processing validation
    • Removed explicit zero check
    • Maintained core functionality with fewer checks

6. Test Suite Updates

  • Added _unspentPersonalRoundCaps as class variable
  • Updated all contribution test calls with empty unspent caps array
  • Commented out obsolete tests:
    • testContributeToRoundFor_revertsGivenUnspentCapsWithNonContiguousRoundIds
    • testContributeToRoundFor_revertsGivenUnspentCapsRoundIdsNotStrictlyIncreasing
    • testContributeToRoundFor_allModeWithGlobalStartRestrictsTotalAccumulation
  • Updated error assertions to use new consolidated errors
  • Removed redundant eligibility checks in tests

7. Gas Optimizations

  • Removed duplicate storage reads
  • Consolidated multiple checks into single operations
  • Simplified loop conditions
  • Removed redundant state tracking
  • Optimized event parameter storage

8. Interface Changes

// Old
function contributeToRoundFor(
    address user_,
    uint32 roundId_,
    uint amount_,
    uint8 accessCriteriaId_,
    bytes32[] calldata merkleProof_
) external;

// New (Single consolidated function)
function contributeToRoundFor(
    address user_,
    uint32 roundId_,
    uint amount_,
    uint8 accessCriteriaId_,
    bytes32[] memory merkleProof_,
    UnspentPersonalRoundCap[] calldata unspentPersonalRoundCaps_
) external;

Migration Guide

For Contract Users

  1. Update all contribution calls to include empty unspent caps array:
// Old
fundingPot.contributeToRoundFor(user, roundId, amount, accessId, proof);

// New
fundingPot.contributeToRoundFor(
    user, 
    roundId, 
    amount, 
    accessId, 
    proof,
    new UnspentPersonalRoundCap[](0)
);
  1. Update error handling:
// Old
try {
    // ... contribution code ...
} catch (bytes memory lowLevelData) {
    if (abi.decodeWithSelector(lowLevelData) == Module__LM_PC_FundingPot__RoundStartMustBeInFuture.selector) {
        // handle specific error
    }
}

// New
try {
    // ... contribution code ...
} catch (bytes memory lowLevelData) {
    if (abi.decodeWithSelector(lowLevelData) == Module__LM_PC_FundingPot__RoundParamsInvalid.selector) {
        // handle consolidated error
    }
}
  1. Remove any dependency on getUserEligibility:
// Old
(bool eligible, uint remaining) = fundingPot.getUserEligibility(roundId, accessId, proof, user);

// New
// Implement your own eligibility check or handle it during contribution

For Contract Implementers

  1. Update error handling to use consolidated errors
  2. Remove any assumptions about round ID ordering in unspent caps
  3. Update event handling for AllowlistedAddressesRemoved
  4. Update tests to use new function signatures and error types

Testing

  • All existing tests have been updated and pass

Security Considerations

  • Removed redundant checks should not impact security
  • More tests should be written now that these checks are removed to make sure unspent caps can't be reused
  • Tests and functions need to be validated for bugs and edge cases

totalAggregatedPersonalCap += privileges.personalCap;
}
// Mark the specific caps that were used in this contribution
usedUnspentCaps[user_][currentProcessingRoundId][roundCapInfo.accessCriteriaId] = true;

Choose a reason for hiding this comment

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

So this will "spend" the full allowance left even if there was more allowance to use. This might be acceptable if the user is aware and in control, but if anyone can contribute on behalf of a user, this allows them to contribute the minimum but still "spend" the full allowance the user had.

@0xNuggan 0xNuggan merged commit 32df604 into InverterNetwork:feat/funding-pot Sep 16, 2025
4 of 5 checks passed
0xNuggan added a commit that referenced this pull request Sep 17, 2025
* init: create the funding pot v1 template

* feat: funding pot admin authorization

* test: add tests for funding pot contract

* feat: add checks for granting and revoking roles

* feat: add funding pot admin role check

* test: add gherkin guide and unit tests

* fix: add missing internal function to exposed_

* chore: remove redundant template code

* test: remove unused test code

* test: code format and unit tests

* chore: apply PR review changes

* Test: Add test for internal function

* chore: remove redundant code

* chore: inverter standard change

* Feat: Add round creation and edit functions

* feat: create and edit rounds implementation

* test: add tests for the edit rounds functionality

* fix: follow inverter standard and remove redundant code

* fix: follow inverter standard and remove redundant code

* fix: follow inverter standard and remove redundant code

* feat: add access criteria implementation

* test: code format & access control criteria unit tests

* fix: inverter standard changes

* fix: add bool to getRoundAccessCriteria()

* fix: inverter standard changes

* fix: inverter standard changes

* fix: PR Review Fix#1

* fix: PR Review Fix#2

* fix: PR Review Fix#3

* fix: PR Review Fix#4

* fix:remove helper functions, fix stack too deep

* fix:support interface issue and redundant param variables

* chore: fmt

* feat: setAccessCriteriaPrivilages() implementation

* feat: update round contribution functionality

* test: add unhappy paths for open access criteria

* test: add unhappy paths for NFT, merkle root and allowed list access criteria

* test: add all unhappy paths

* test: add happy paths based on the technical specifications

* fix:update code based on internal review

* fix:initialise contributing token properly

* feat: implement personal cap based on access criteria

* add exposed functions

* test: add fuzz tests for internal contribution functions

* rebase changes

* add closure logic

* fix: fix broken tests

* fix:fix all failing test

* fix:rename accessId to accessCriteriaId

* fix: fix warning

* fix:remove unecessary helper function

* fix:add tests for exposed functions

* test: add gherkin comments to contribution tests

* fix:add todos for jeffrey

* feat: change allowed addresses array to mapping in AccessCriteria struct

* fix: PR Review Fix#1

* fix: PR Review Fix#2

* fix: fix the personal cap rollover issue

* fix: remove redundant code

* fix: PR Review Fix#3

* fix: PR Review Fix#4

* PR Review Fix#5

* fix: git rebase

* test: add unit tests closeRound

* fix: code format and unit tests

* fix: mock the bonding curve token

* fix: PR Review fixes#1

* fix: PR Review fixes#2

* fix: accessId unit test bug

* fix: PR Review fixes#3

* fix: PR Review fixes#4

* fix: unit test closeround via mock

* fix: PR Review Fix

* fix: PR Review Fix: add batches for creating payment orders

* fix: fix stack too deep error

* PR Review Fix: format and code cleanup

* fix:gas/contract optimziations

* fix: PR feedback

* fix: PR Review

* chore: `contributeToRound` => `contributeToRoundFor`

* fix: consistent uint64 for roundId

* chore: formatting

* fix: remove mock contract

* fix: remove file

* chore: remove unused variables

* fix: e2e test debug

* test: Funding Pot E2E test

* fix: minor bug fixes

* chore: rebase

* test: add e2e test

* fix: add lifecycle test and fix set up

* fix: add notes to e2e

* fix: compile issues

* chore: e2e test closeround

* test: improve test coverage

* chore: merge branch contribute and close functionality

* fix: fix merge changes

* chore: rebase contribute-close onto dev

* fix: fix rebase issues

* fix: fix failing test

* tests: improve test coverage

* chore: code format

* chore: code format & fix minor bugs

* chore: Remove stale/irrelevant values in set/edit roundAccessCriteria

* chore: add support for multiple access control criteria

* fix: fix double underscore styling issue

* chore: change 'accessId' to 'accessCriteriaId' in the test file

* chore: convert necessary getter functions from internal to external

* fix: add missing assertion for e2e test

* fix: add optimization to reduce contract size

* feat: accumulation mode

* chore: adds tests for accumulation mode behavior

* feat: globally configurable start for accumulation logic

* fix: mode-specific logic & tests

* chore: contract size excess 879 -> 239

* chore: custom compiler settings for FP

* fix: adds missing additional_compiler_profiles

* fix: max personal cap defined by highest access criteria

* chore: test structure cleanup

* chore: fmt

* chore: removes redundant code

* fix: compiler warnings

* chore: remove redundant check on zero contribution

* chore: fmt

* fix: `>=` instead of `==`

* fix: merge conflicts

* fix: audit fixes #1

* fix: audit fixes 2

* Feat/contract resizing (#769)

* fix: new audit fixes 1

* fix: new audit fixes 2

* fix: new audit fixes 3

* fix: audit fixes 4

* fix: new audit fixes 4 -remove unneccessary code

* fix: new audit fixes 5

* fix: new audit fixes 5: add getter function

* fix: contract size fix 1

* fix: contract size fix 2: remove getter functions and make public

* fix: contract size fix 3

* fix: contract size fix 3: if condition

* contract resizing

* chore:remove uncommented tests

* chore:remove hook validation

---------

Co-authored-by: Zuhaib Mohammed <zzzuhaibmohd@gmail.com>
Co-authored-by: Jeffrey Owoloko <72028836+JeffreyJoel@users.noreply.github.com>
Co-authored-by: JeffreyJoel <jowoloko@gmail.com>

* chore: format

---------

Co-authored-by: Zuhaib Mohammed <zzzuhaibmohd@gmail.com>
Co-authored-by: JeffreyJoel <jowoloko@gmail.com>
Co-authored-by: Leandro Faria <lee.lara1219@gmail.com>
Co-authored-by: leeftk <40748420+leeftk@users.noreply.github.com>
Co-authored-by: Jeffrey Owoloko <72028836+JeffreyJoel@users.noreply.github.com>
Co-authored-by: 0xNuggan <82726722+0xNuggan@users.noreply.github.com>
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.

5 participants