feat(StakeManager): add capabilities to register vaults #71

0x-r4bbit · 2024-10-25T08:26:47Z

This commit introduces changes related to vault registrations in the stake manager.

The stake manager needs to keep track of the vaults a users creates so it can aggregate accumulated MP across vaults for any given user.

The StakeVault now comes with a register() function which needs to be called to register itself with the stake manager. StakeManager has a new onlyRegisteredVault modifier that ensures only registered vaults can actually stake and unstake.

Closes #70

Checklist

Ensure you completed all of the steps below before submitting your pull request:

Added natspec comments?
Ran pnpm adorno?
Ran pnpm verify?

0x-r4bbit · 2024-10-25T08:29:58Z

src/StakeVault.sol

+     */
+    function register() external {
+        stakeManager.registerVault();
+    }


First I wanted to make register() part of the StakeVault's constructor() but the problem is registerVault() has the trusted code hash modifier. We need to create a StakeVault "template" instance first to register its codehash with the Stake manager, but the instantiation then fails due to registerVault() reverting.

I then decided to move this out and add an onlyRegisteredVaults modifier to the sensitive functions.

This should ensure StakeVault will call register() before they'll interact with the StakeManager.

Since we only allow for staking once per vault, we could also make this part of stake() instead and enforce registration implicitly there.

Let me know what you guys think.

Since we only allow for staking once per vault, we could also make this part of stake() instead and enforce registration implicitly there.

I like this idea!

Otherwise we can do it in the constructor anyway since we know the bytecode already, we don't need to deploy a real one before. But anyway I like it if we do it in the stake function

Otherwise we can do it in the constructor anyway since we know the bytecode already, we don't need to deploy a real one before. But anyway I like it if we do it in the stake function

So I gave it a bit more thought.
When I started implementing it, it felt odd to me that stake() would create a side effect (registration).
Now I'm kind of hesitant..

re: the other point: we can't do it in the constructor because we don't know the deployed byte code at this point in time. This is because of the STAKING_TOKEN in stakevault being immutable, making it part of the deploycode.

We know the staking token is going to be SNT on mainnet/l2 but for local testing it's always a different one.

So I don't think we can make registration part of the constructor.

because we don't know the deployed byte code at this point in time

@0x-r4bbit we can add a script that compute it in local. It can use a mock manager without modifier so that the vault can be deployed in local and the bytecode can be printed out. after that we know it and we can use it in production. we don't need to deploy a contract in prod to know the bytecode, wdyt?

we don't need to deploy a contract in prod to know the bytecode, wdyt?

I think that would be idea.
If we do a minimal proxy clone pattern for StakeVault that needs a template deployed anyways, I think we don't need to set up a script for that either.

Maybe another option worth exploring?

0x-r4bbit · 2024-10-25T08:31:42Z

src/StakeVault.sol

+     */
+    function owner() public view override(Ownable, IStakeVault) returns (address) {
+        return super.owner();
+    }


In StakeManager.registerVault() we're reading StakeVault.owner() so I added owner() to the IStakeVault interface.

Needed to explicitly add owner() here to make this compile.

src/RewardsStreamerMP.sol

0x-r4bbit · 2024-10-30T13:24:01Z

@gravityblast I've updated the PR according to your comments, accept for #71 (comment)

3esmit

This should be in another contact.

0x-r4bbit · 2024-12-01T10:22:07Z

@3esmit moved vault registration into separate contract

db96cd7

3esmit · 2024-12-02T15:12:06Z

src/RewardsStreamerMP.sol

+     * @param vault The address of the vault to check
+     * @return true if the vault is registered, false otherwise
+     */
+    function isVaultRegistered(address vault) public view returns (bool) {


I don't think this belongs here.

StakeManager should not care about this.

Yeap, this is a leftover!

3esmit · 2024-12-02T15:13:48Z

src/RewardsStreamerMP.sol

+    /**
+     * @notice Get all vaults owned by an address
+     */
+    function getUserVaults(address owner) external view returns (address[] memory) {


I don't think this belongs here, StakeVault should interact directly with Registry, and systems that want to read the registry should interact directly with Registry.

Same here, left over! Will remove

3esmit · 2024-12-02T15:14:02Z

src/RewardsStreamerMP.sol

+     * @param user The address of the user
+     * @return The total maximum multiplier points for the user
+     */
+    function getUserTotalMaxMP(address user) external view returns (uint256) {


This should be in registry as well

Hm.. not sure about that...
So far we said that the staking protocol ultimately is one of the "XP providers" for the XP token contract.
Therefore, the stakemanager has to expose these. StakeManager is also where rewards are distributed.

I think it makes sense to keep the registry solely as registry for "which account owns which vaults".

The StakeManagerRegistry could be the source of XP, while the StakeManager is only the logic. If forwarding of calls are needed they can be done in StakeManagerRegistry, not in StakeManager.

At least this is what makes sense for me, from a logical flow of information.

There's a tradeoff:

Do users/apps have to worry about that there's a registry under the hood or not.

With the current implementation, calls always go through the stake manager and it knows whether or not it has to talk to a registry that users don't have to know about.

It also ensures that StakeVaults that interact with the system are whitelisted.

The path you're suggesting means:

Registry has to whitelist as well, because we need to be sure only legit stakevault interact with the system

Apps have to talk to one more contract to figure things out

I'm personally in favor of the former, but if there's majority consensus I'm happy to make the changes.

@gravityblast maybe you can leave your opinion here as well?

I would keep the functions in the staking manager and keep the registry only as a registry of vaults

3esmit · 2024-12-02T15:15:07Z

src/RewardsStreamerMP.sol

+        external
+        onlyTrustedCodehash
+        onlyNotEmergencyMode
+        onlyRegisteredVault


Why is a requirement to have the vault registered? Is there any security benefits on that? For me it seems is just a tool. This type of verification can be done in UI, and there should be no problem in using StakeManger without register.

I'd say, it's an enforced invariant that only registered vaults can stake.
If it's not enforced here, then any vault could stake and the vault might not be registered, meaning it would not be taken into consideration when querying the XP for an owner of vaults.

Ok, but it does not pose any risk for the current system, this should be a requirement by the system which requires this.
Otherwise the dependency graph is weird.

this should be a requirement by the system which requires this.

Can you elaborate what you're looking for? Having this modifier is essentially that requirement, no?

and there should be no problem in using StakeManger without register.

So, to clarify here: the reason we've introduced registration of vaults by owners, is so that we can aggregate all MP/XP/stakedBalance for a given owner.

If a StakeVault is not registered, it won't be known in the aggregation of XP.

Sure, we could say: Well, if a user ignores the rules and still stakes without registering, then it's their problem.

I think though, we shouldn't even allow that.

I agree with keeping the registration of the vaults, it feels more consistent like this

gravityblast · 2024-12-03T13:12:25Z

src/RewardsStreamerMP.sol

-    function _updateAccountMP(address accountAddress) internal {
-        Account storage account = accounts[accountAddress];
-
+    function _getAccruedMP(Account storage account) internal view returns (uint256) {


very small thing, shall we call this _getAccountAccruedMP so that it's clear it's for the account? just in case we extract the global MP logic to something with the same name

I agree with renaming this.

I think we should do this across the board for all other things as well #87

3esmit · 2024-12-03T14:12:17Z

The registry contract should have all the logic related to registry and reading balances, otherwise it does not make sense to have a separate contract.

Personally I prefer systems to have a strong single responsibility logic, whenever possible, because it leads to code that is easier to maintain in the long run.

I'll explore more into the both designs below.

A) StakeManager with Embedded Registry Logic

flowchart TD
    subgraph User
        StakeVault
    end
    StakeVault --> StakeManager
    XPToken --> StakeManager

Benefits:

Simplicity in Interactions:
- Fewer Contracts: With the registry embedded, there's one less contract to deploy and interact with.
- Simplified Calls: External contracts like XP Token interact with a single contract to get all necessary data.
Efficiency:
- Reduced Cross-Contract Calls: Fewer interactions between contracts can lead to lower gas costs.
- Data Access: Aggregated data is readily available within StakeManager, avoiding multiple external calls.
Centralized Data Management:
- Single Source of Truth: All relevant data is maintained in one place, reducing the risk of data inconsistencies.
- Easier Synchronization: Updates to MPs and vault registrations occur within the same contract.
Performance:
- Optimized Operations: Internal function calls are cheaper than external calls, potentially improving performance

Drawbacks:

Violation of Single Responsibility Principle (SRP):
- Mixed Concerns: StakeManager handles both staking logic and registry management, which are distinct responsibilities.
- Increased Complexity: Combining multiple roles can make the contract larger and harder to understand.
Maintainability Issues:
- Tight Coupling: Changes to registry logic could inadvertently affect staking logic and vice versa.
- Difficult to Test: More extensive testing is required as changes can have wider implications.
Security Risks:
- Larger Attack Surface: A bigger contract with more functions increases the potential for vulnerabilities.
- Risk of Exploits: Bugs in the registry functionality could compromise staking operations.
Reduced Flexibility:
- Harder to Extend: Adding new features specific to the registry or staking logic becomes more complex.
- Less Modular: Cannot reuse the registry functionality independently in other contexts.

B) Strong Single Responsibility Principle (SRP) with Separate Registry

In this approach, the Registry is a separate contract responsible solely for mapping users to their StakeVaults. The StakeManager focuses exclusively on staking logic and MPs.

flowchart TD
    subgraph User
        StakeVault
    end
    StakeVault --> StakeManager
    StakeVault -->|register| Registry
    XPToken -->|balanceOfAccount| Registry
    Registry -->|balanceOfVault| StakeManager

Benefits:

Adherence to SRP:
- Clear Separation of Concerns: Each contract has a single, well-defined responsibility.
- Improved Readability: Code is easier to understand when each contract handles a specific aspect.
Enhanced Maintainability:
- Independent Changes: Modifications in the Registry do not affect the StakeManager and vice versa.
- Simplified Testing: Easier to write focused tests for each contract.
Better Security Posture:
- Smaller Contracts: Less code per contract reduces the chance of vulnerabilities.
- Isolation of Functions: A flaw in one contract is less likely to impact others.
Flexibility and Reusability:
- Modular Design: The Registry can be reused or replaced without altering the staking logic.
- Easier to Extend: New features can be added to one component without affecting others.
Scalability:
- Distributed Load: Workload can be distributed across contracts, which can be beneficial if certain functions become more complex.

Drawbacks:

Increased Complexity in Interactions:
- Multiple Contracts: More contracts to deploy, manage, and interact with.
- Complex Call Chains: External contracts like XP Token need to interact with both Registry and StakeManager.
Higher Gas Costs:
- Cross-Contract Calls: Interactions between contracts are more expensive than internal calls.
- Aggregation Overhead: XP Token may need to fetch data from multiple sources.
Data Synchronization Challenges:
- Consistency Issues: Ensuring all contracts have up-to-date data requires careful management.
- Atomicity: Transactions involving multiple contracts may be harder to make atomic.
Deployment and Management Overhead:
- More Contracts to Deploy: Additional steps in deployment and potential points of failure.
- Version Compatibility: Ensuring all contracts remain compatible during upgrades.

Key Takeaways:

Embedded Registry Approach:
- Simpler external interactions.
- Harder to maintain.
- Potentially lower gas costs.
- Violates SRP by combining concerns.
Separate Registry Approach:
- Adheres to SRP with modular design.
- Easier to maintain.
- More complex interactions.
- Potentially higher gas costs due to cross-contract calls.

This commit introduces changes related to vault registrations in the stake manager. The stake manager needs to keep track of the vaults a users creates so it can aggregate accumulated MP across vaults for any given user. The `StakeVault` now comes with a `register()` function which needs to be called to register itself with the stake manager. `StakeManager` has a new `onlyRegisteredVault` modifier that ensures only registered vaults can actually `stake` and `unstake`. Closes #70

0x-r4bbit · 2024-12-03T15:41:41Z

As discussed offline, we're moving ahead with not having a separate registry contract.
I've removed the commits from the PR that introduced it.

The reasoning is that, at least now, it introduces unnecessarily complexity.
We'll revisit this in the future if we think this poses a problem.

Once CI is green this will be merged.

0x-r4bbit · 2024-12-03T15:54:48Z

All checks successful. @3esmit I'm dismissing your review so this can be merged (as it's currently blocking otherwise).
Please don't take it personally :D

We've ultimately agreed on not implementing a separate registry contract.

0x-r4bbit requested review from gravityblast and 3esmit October 25, 2024 08:26

0x-r4bbit commented Oct 25, 2024

View reviewed changes

0x-r4bbit mentioned this pull request Oct 25, 2024

Allow for data retrieval of individual users #70

Closed

0x-r4bbit force-pushed the feat/register-vault branch from 80ff390 to d6abc16 Compare October 25, 2024 08:37

0x-r4bbit commented Oct 25, 2024

View reviewed changes

src/RewardsStreamerMP.sol Show resolved Hide resolved

0x-r4bbit force-pushed the feat/register-vault branch 2 times, most recently from 4fe58c2 to 1564bd2 Compare October 25, 2024 09:44

gravityblast reviewed Oct 29, 2024

View reviewed changes

src/RewardsStreamerMP.sol Outdated Show resolved Hide resolved

gravityblast reviewed Oct 29, 2024

View reviewed changes

src/RewardsStreamerMP.sol Outdated Show resolved Hide resolved

gravityblast approved these changes Oct 29, 2024

View reviewed changes

0x-r4bbit force-pushed the feat/register-vault branch from 1564bd2 to 0bdfd2b Compare October 30, 2024 13:23

0x-r4bbit force-pushed the feat/register-vault branch 3 times, most recently from 89c702d to 3c47f85 Compare November 29, 2024 17:23

3esmit previously requested changes Nov 29, 2024

View reviewed changes

0x-r4bbit force-pushed the feat/register-vault branch from 3c47f85 to db96cd7 Compare December 1, 2024 10:16

0x-r4bbit requested a review from 3esmit December 1, 2024 10:21

3esmit reviewed Dec 2, 2024

View reviewed changes

0x-r4bbit force-pushed the feat/register-vault branch 3 times, most recently from 45d4407 to c8ea850 Compare December 3, 2024 13:08

gravityblast reviewed Dec 3, 2024

View reviewed changes

gravityblast approved these changes Dec 3, 2024

View reviewed changes

0x-r4bbit mentioned this pull request Dec 3, 2024

Update account terminology in contracts #87

Open

0x-r4bbit force-pushed the feat/register-vault branch from c8ea850 to d61a3e3 Compare December 3, 2024 13:39

0x-r4bbit force-pushed the feat/register-vault branch from d61a3e3 to 3dd4668 Compare December 3, 2024 15:40

0x-r4bbit merged commit 9374025 into main Dec 3, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(StakeManager): add capabilities to register vaults #71

feat(StakeManager): add capabilities to register vaults #71

0x-r4bbit commented Oct 25, 2024 •

edited

Loading

0x-r4bbit Oct 25, 2024

0x-r4bbit Oct 25, 2024

gravityblast Oct 29, 2024

0x-r4bbit Oct 30, 2024

gravityblast Oct 30, 2024

0x-r4bbit Oct 30, 2024

0x-r4bbit Oct 25, 2024

0x-r4bbit commented Oct 30, 2024

3esmit left a comment

0x-r4bbit commented Dec 1, 2024

3esmit Dec 2, 2024

0x-r4bbit Dec 2, 2024

3esmit Dec 2, 2024

0x-r4bbit Dec 2, 2024

3esmit Dec 2, 2024

0x-r4bbit Dec 2, 2024

3esmit Dec 2, 2024

0x-r4bbit Dec 3, 2024

gravityblast Dec 3, 2024

3esmit Dec 2, 2024

0x-r4bbit Dec 2, 2024

3esmit Dec 2, 2024

0x-r4bbit Dec 2, 2024

0x-r4bbit Dec 3, 2024

gravityblast Dec 3, 2024

gravityblast Dec 3, 2024

0x-r4bbit Dec 3, 2024

3esmit commented Dec 3, 2024

0x-r4bbit commented Dec 3, 2024

0x-r4bbit commented Dec 3, 2024

feat(StakeManager): add capabilities to register vaults #71

feat(StakeManager): add capabilities to register vaults #71

Conversation

0x-r4bbit commented Oct 25, 2024 • edited Loading

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0x-r4bbit commented Oct 30, 2024

3esmit left a comment

Choose a reason for hiding this comment

0x-r4bbit commented Dec 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

3esmit commented Dec 3, 2024

A) StakeManager with Embedded Registry Logic

Benefits:

Drawbacks:

B) Strong Single Responsibility Principle (SRP) with Separate Registry

Benefits:

Drawbacks:

Key Takeaways:

0x-r4bbit commented Dec 3, 2024

0x-r4bbit commented Dec 3, 2024

0x-r4bbit commented Oct 25, 2024 •

edited

Loading