Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow for data retrieval of individual users #70

Closed
0x-r4bbit opened this issue Oct 24, 2024 · 2 comments · Fixed by #71
Closed

Allow for data retrieval of individual users #70

0x-r4bbit opened this issue Oct 24, 2024 · 2 comments · Fixed by #71
Assignees
@0x-r4bbit
Labels
commitment: staking-protocol-v1 task: xp-nft-v1:implementation
Milestone
staking-protocol-v1

Comments

@0x-r4bbit
Copy link
Collaborator

In #62 we made a change that underlined the fact that the staking protocol doesn't really know anything about users.

However, we've realized that there's an issue:

  1. We have XPNFTToken which uses XPToken to render any given user's XP balance
  2. The NFTs are soul bound. Any address in existence owns such an NFT
  3. The staking protocol keeps track of "accounts" that stake in the system, these accounts are StakeVaults
  4. One actual user can have multiple StakeVaults

If some user X wants to see their XP token balance in their XP NFT, they'll have a balance of 0 because it's not their EOA (or smart account) that is registered with the staking system, but rather the StakeVaults.

We want the users to be able to see their XP balance that is also determined by their stake amount, so somewhere in the system, there has to be a connection between the users and the vaults they are creating/registering with the system.

One way to go about this is to reintroduce some notion of VaultOwner in the staking protocol that keeps track of each vault that an owner has created:

    // StakeManager
    mapping(address owner => address[] vaults) public userVaults;
    function registerVault() external onlyTrustedCodehash {
        address vault = msg.sender;
        address owner = StakeVault(vault).owner();
        
        if (vaultOwners[vault] != address(0)) {
            revert StakingManager__VaultAlreadyRegistered();
        }

        // Verify this is a legitimate vault by checking it points to us
        try StakeVault(vault).stakeManager() returns (address manager) {
            if (manager != address(this)) {
                revert StakingManager__InvalidVault();
            }
        } catch {
            revert StakingManager__InvalidVault();
        }

        vaultOwners[vault] = owner;
        userVaults[owner].push(vault);
    }

    function getUserTotalMP(address user) external view returns (uint256) {
        address[] memory vaults = userVaults[user];
        uint256 totalUserMP;
        
        for (uint256 i = 0; i < vaults.length; i++) {
            Account memory account = accounts[vaults[i]];
            totalUserMP += account.accountMP;
        }
        
        return totalUserMP;
    }

Something along those lines.

The StakeVault could then register itself during instantiation:

    constructor(address _owner, IStakeManager _stakeManager) Ownable(_owner) {
        STAKING_TOKEN = _stakeManager.STAKING_TOKEN();
        stakeManager = _stakeManager;
        
        // Register this vault with the StakeManager
        _stakeManager.registerVault();
    }

One thing that could potentially be an issue here is that an account could create many stake vaults and register them without actually staking.
With enough vaults, getUserTotalMP could run into a block gas limit.

One thing we could do to reduce the likelyhood is to only register with the StakeManager when a vault is actually calling stake() as this would require users to also add stake into the system.
@0x-r4bbit 0x-r4bbit added this to the xp-nft-v1 milestone Oct 24, 2024
@TheMarvelFan
Copy link

Hi @0x-r4bbit ,

I have thought of a few solutions for this problem.

  1. We could impose a cap on the number of StakeVaults any single user can create. This would prevent the block gas limit issue.
  2. We could modify the registration process to only count active vaults. A vault would be considered "active" if it contains a stake or contributes to the user's total XP.
  3. Instead of summing balances across all vaults in real-time, we could aggregate the user's total XP balance in a separate variable. Each time a vault's XP or stake changes, the aggregated value is updated.

If you like any of these suggestions, I can create a PR for implementing it.

@0x-r4bbit 0x-r4bbit moved this to In Progress in Tasks - Smart Contracts Oct 25, 2024
@0x-r4bbit 0x-r4bbit mentioned this issue Oct 25, 2024
Merged
3 tasks
@0x-r4bbit
Copy link
Collaborator Author

@TheMarvelFan Hey man!

Thank you for showing your interest in contributing to this project.
While you were writing this I was already working on this. There's a pending PR now at #71

Feel free to take a look and leave your feedback there!

0x-r4bbit added a commit that referenced this issue Oct 25, 2024
@0x-r4bbit
feat(StakeManager): add capabilities to register vaults
d6abc16 
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 0x-r4bbit self-assigned this Oct 26, 2024
0x-r4bbit added a commit that referenced this issue Oct 30, 2024
@0x-r4bbit
feat(StakeManager): add capabilities to register vaults
d80a6f0 
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 added a commit that referenced this issue Nov 29, 2024
@0x-r4bbit
feat(StakeManager): add capabilities to register vaults
9d45b64 
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 added a commit that referenced this issue Dec 1, 2024
@0x-r4bbit
feat(StakeManager): add capabilities to register vaults
e6774d8 
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 added a commit that referenced this issue Dec 3, 2024
@0x-r4bbit
feat(StakeManager): add capabilities to register vaults
c961010 
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 added a commit that referenced this issue Dec 3, 2024
@0x-r4bbit
feat(StakeManager): add capabilities to register vaults
3dd4668 
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
@github-project-automation github-project-automation bot moved this from In Progress to Done in Tasks - Smart Contracts Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@0x-r4bbit 0x-r4bbit

Labels
commitment: staking-protocol-v1 task: xp-nft-v1:implementation
Projects
Tasks - Smart Contracts
Status: Done
Milestone
staking-protocol-v1
Development

Successfully merging a pull request may close this issue.

feat(StakeManager): add capabilities to register vaults vacp2p/staking-reward-streamer
2 participants
@0x-r4bbit @TheMarvelFan