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.

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

[SI] VaultHubViewer contract #905

Open
wants to merge 7 commits into
base: feat/vaults
Choose a base branch
from
Open

[SI] VaultHubViewer contract #905

wants to merge 7 commits into from

Conversation

DiRaiks
Copy link
Contributor

@DiRaiks DiRaiks commented Dec 23, 2024

A short summary of the changes.

Context

This PR introduces the VaultHubViewerV1 contract for read-only interactions with the VaultHub. It provides methods to:

  • Check vault ownership: isOwner verifies if an address is the owner of a vault.
  • Check role assignments: isHasRole checks if an address holds a specific role in a vault.
  • Retrieve vaults by ownership: vaultsByOwner returns vaults owned by a given address.
  • Retrieve vaults by role: vaultsByRole returns vaults where an address holds a specified role.
  • Filter zero vaults: _filterNonZeroVaults ensures only valid vaults are included.

The contract works with the IVaultHub interface. It also includes the ZeroArgument error for invalid zero address inputs.

Key Features:

  • Immutable vaultHub address.
  • Non-state-changing queries for vault information.

This contract offers a simple, efficient way to query vaults and roles without affecting the state.

Problem

What problem this PR solves, link relevant issue if it exists

Solution

Your proposed solution

@DiRaiks DiRaiks requested review from a team as code owners December 23, 2024 13:17
@DiRaiks DiRaiks changed the base branch from master to feat/vaults-suggestions December 23, 2024 13:17
Copy link
Member

@tamtamchik tamtamchik left a comment

Choose a reason for hiding this comment

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

👀 Added some comments for further optimizations

contracts/0.8.25/vaults/VaultHubViewerV1.sol Outdated Show resolved Hide resolved
}

interface IVault is IStakingVault {
function owner() external view returns (address);
Copy link
Member

Choose a reason for hiding this comment

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

It could be worth moving inside IStakingVault. Looks like may be used often.

Copy link
Contributor

Choose a reason for hiding this comment

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

owner here conflicts when moving to IStakingVault, because StakingVault is IStakingVault, ..., Ownable

contracts/0.8.25/vaults/VaultHubViewerV1.sol Outdated Show resolved Hide resolved
contracts/0.8.25/vaults/VaultHubViewerV1.sol Show resolved Hide resolved
contracts/0.8.25/vaults/VaultHubViewerV1.sol Outdated Show resolved Hide resolved
contracts/0.8.25/vaults/VaultHubViewerV1.sol Outdated Show resolved Hide resolved
contracts/0.8.25/vaults/VaultHubViewerV1.sol Outdated Show resolved Hide resolved
contracts/0.8.25/vaults/VaultHubViewerV1.sol Outdated Show resolved Hide resolved
contracts/0.8.25/vaults/VaultHubViewerV1.sol Outdated Show resolved Hide resolved
Base automatically changed from feat/vaults-suggestions to feat-immutable-operator-in-vault December 24, 2024 15:52
Base automatically changed from feat-immutable-operator-in-vault to feat/vaults December 24, 2024 15:53
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.

3 participants