feat: first draft for upgrades #140

sogipec · 2025-10-15T17:29:52Z

No description provided.

Copilot

Pull Request Overview

This PR updates the Merkl protocol with significant changes to support upgrades and new operator functionality. The changes primarily involve updating author attribution from "Angle Labs, Inc." to "Merkl SAS" and implementing a comprehensive operator system for campaign and reward management.

Key changes include:

Introduction of main operators and creator allowances for delegated campaign management
Enhanced claim recipient functionality with governance support
Removal of deprecated signature verification and distribution creation methods

Reviewed Changes

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

Show a summary per file

File	Description
contracts/partners/tokenWrappers/SonicFragment.sol	Updated author attribution to Merkl SAS
contracts/partners/tokenWrappers/PointToken.sol	Updated author attribution to Merkl SAS
contracts/mock/DistributionCreatorUpdatable.sol	Updated author attribution to Merkl SAS
contracts/interfaces/IAccessControlManager.sol	Updated author attribution to Merkl SAS
contracts/Distributor.sol	Added main operator system and enhanced claim recipient functionality
contracts/DistributionCreatorWithDistributions.sol	New contract for backward compatibility with deprecated distribution model
contracts/DistributionCreator.sol	Major refactoring with operator allowances and removal of deprecated functionality
contracts/AccessControlManager.sol	Updated author attribution to Merkl SAS
LICENSE	Updated licensor information to Merkl SAS

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

contracts/Distributor.sol

contracts/DistributionCreator.sol

GuillaumeNervoXS

I didn't check the distributor and deployment scripts yet

contracts/partners/tokenWrappers/BaseTokenWrapper.sol

GuillaumeNervoXS · 2025-10-23T12:50:45Z

scripts/DistributionCreator.s.sol

    }
 }

-// Sign script


why did you remove this one and the one to create campaigns ?

Added it back

contracts/DistributionCreator.sol

GuillaumeNervoXS · 2025-10-23T16:03:32Z

contracts/DistributionCreator.sol

    }

-    /// @notice Gets all the campaigns which were live at some point between `start` and `end` timestamp
-    /// @param skip Disregard distibutions with a global index lower than `skip`


Why did we let the functions linked to the old distributions type the last time? Wasn't it because a protocol relied on it?

put it on another file

GuillaumeNervoXS · 2025-10-23T17:09:33Z

contracts/DistributionCreator.sol

+        address rewardToken,
+        address to,
+        uint256 amount
+    ) external onlyUserOrGovernor(user) {


do we really need this one with governor rights instead of just user amd operator?
The less we are liable the better I'd say

More as a safeguard if accounting is broken in some way here

Same concern as Guillaume.
If accounting is broken, we could upgrade if needed.
If client gets pwned, he will ask for us to handle this, which I don't think we want to do ?

In terms of security, on our side this exposes the funds on creator to the security of multisig.
If we removed the Governor modifier here, we should also kill recoverFees functionnality as well.

Indifferent, I still think we're good to leave it as is. It's more a sales stance and positioning of saying we won't do it

I think it is not worth it, I don't see how/why we would be more reactive than a user

GuillaumeNervoXS · 2025-10-23T17:12:17Z

contracts/DistributionCreator.sol

+
+    /// @notice Toggles the ability of an `operator` to manage campaigns on behalf of a `user`
+    /// @dev Only the user themselves or a governor can call this function
+    function toggleCampaignOperator(address user, address operator) external onlyUserOrGovernor(user) {


why not just having the mapping 'creatorAllowance' and if allowance non null --> valid operator

for the sake of composability and having different addresses possible, like most people may not split roles, but seems cleaner to have different roles

contracts/DistributionCreatorWithDistributions.sol

contracts/DistributionCreator.sol

nlecoufl · 2025-10-31T06:53:46Z

contracts/Distributor.sol

            // The recipient set in the context of the call to `claim` can override the default recipient set by the user
            if (msg.sender != user || recipient == address(0)) {
                address userSetRecipient = claimRecipient[user][token];
+                if (userSetRecipient == address(0)) userSetRecipient = claimRecipient[user][address(0)];


A scenario that can happen:

First, a user set an address 0xA as claimRecipient for token A

Then, he finally set an address 0xB as default recipient for all tokens (using 0x0…0)

Finally, when a claim is triggered, claimRecipient[user][token] equal 0xA. Thus tokens will not flow to 0xB (as some users will expect I guess, because setting a default recipient for all tokens was their last action on-chain)

I wonder if we want to keep this behavior or not.

Yes in this setup, by default token recipient is less senior than what you set for a token -> imo this is navigated at the UI level

contracts/DistributionCreator.sol

vincent-merkl · 2025-10-31T10:22:15Z

contracts/DistributionCreator.sol

+        address rewardToken,
+        address to,
+        uint256 amount
+    ) external onlyUserOrGovernor(user) {


Same concern as Guillaume.
If accounting is broken, we could upgrade if needed.
If client gets pwned, he will ask for us to handle this, which I don't think we want to do ?

vincent-merkl · 2025-10-31T10:25:34Z

contracts/DistributionCreator.sol

+        address rewardToken,
+        address to,
+        uint256 amount
+    ) external onlyUserOrGovernor(user) {


In terms of security, on our side this exposes the funds on creator to the security of multisig.
If we removed the Governor modifier here, we should also kill recoverFees functionnality as well.

contracts/DistributionCreator.sol

GuillaumeNervoXS

doesn't cost much to keep this fallback if engine fees do not work well -> not confident removing it if we haven't tested it well enough

Just lisibility of the snmart contract and again there are already some tests as subcampaigns like Pendle/Spectra/stake dao/... reies on it already

GuillaumeNervoXS

yes but then you end up reading twice in storage which is suboptimal

Ok but again gas cost is not as much an issue anymore

Copilot AI review requested due to automatic review settings October 15, 2025 17:29

Copilot AI reviewed Oct 15, 2025

View reviewed changes

contracts/Distributor.sol Outdated Show resolved Hide resolved

contracts/DistributionCreator.sol Outdated Show resolved Hide resolved

contracts/DistributionCreator.sol Show resolved Hide resolved

vincent-merkl reviewed Oct 15, 2025

View reviewed changes

contracts/DistributionCreator.sol Show resolved Hide resolved

sogipec added 12 commits October 20, 2025 18:45

first draft

af5e8c5

fix: compile for the code

5cc1b1b

fixing lint

d166404

fixing tests

f73a1aa

rm: modifier

fd66901

fix: balance system for predeposit

04d8c0c

greater or equal

eb58eaa

fix: gap

e133cd3

fix: balance update

b23e780

adding some tests

fc63e80

fix: tests

74b784a

rebase

01d804c

sogipec force-pushed the contract-upgrades branch from e236add to 01d804c Compare October 20, 2025 16:46

Picodes reviewed Oct 21, 2025

View reviewed changes

contracts/DistributionCreator.sol Outdated Show resolved Hide resolved

sogipec added 4 commits October 21, 2025 18:29

fix: signatures whitelist

4dd8f28

fix: all tests

7374b05

fix: tests

eda0bc1

feat: deployment scripts

6ba9347

GuillaumeNervoXS reviewed Oct 23, 2025

View reviewed changes

sogipec added 2 commits October 24, 2025 12:01

fix: guardian claim

3e5e64b

fix: contracts

37bce27

nlecoufl reviewed Oct 29, 2025

View reviewed changes

contracts/DistributionCreator.sol Outdated Show resolved Hide resolved

nlecoufl reviewed Oct 31, 2025

View reviewed changes

vincent-merkl reviewed Oct 31, 2025

View reviewed changes

fix: comments

aa324fa

GuillaumeNervoXS reviewed Nov 6, 2025

View reviewed changes

sogipec added 8 commits November 12, 2025 09:15

chore: tests for NativeTokenWrapper

80b96df

fix: naming

1502017

pull token wrapper tests

1a3b32c

fix: cleaning scripts and balance

657baec

fix: comments in the code

4586841

fix: tests and compilations

95bfae7

fix: lint

005348e

rm: angle references

cd2126a

feat: first draft for upgrades #140

Are you sure you want to change the base?

feat: first draft for upgrades #140

Conversation

sogipec commented Oct 15, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GuillaumeNervoXS left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nlecoufl Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

GuillaumeNervoXS left a comment

Choose a reason for hiding this comment

Uh oh!

GuillaumeNervoXS left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nlecoufl Oct 31, 2025 •

edited

Loading