Skip to content

Allowlist for Wallet Registry #3826

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Allowlist for Wallet Registry #3826

wants to merge 3 commits into from

Conversation

pdyraga
Copy link
Member

@pdyraga pdyraga commented Mar 5, 2025

The allowlist contract replaces the Threshold TokenStaking contract and is as an outcome of TIP-092 and TIP-100 governance decisions. Staking tokens is no longer required to operate nodes. Beta stakers are selected by the DAO and operate the network based on the allowlist maintained by the DAO. The contract will be integrated with the WalletRegistry and replace calls to TokenStaking.

I have been experimenting with various approaches, and the most extreme one was to remove most of the EcdsaAuthorization logic as well as all TokenStaking.seize calls. This would have cascading effects on tBTC Bridge contracts as they rely on WalletRegistry.seize. That would also require implementing weight decrease delays in the Allowlist, so essentially doing work that is already done in WalletRegistry. Considering the pros and cons, I decided on the least invasive option. The WalletRegistry still thinks in terms of stake authorization, but everything is based on the staking provider's weight as set in the Allowlist, and weight decrease delays are enforced by the existing mechanism in EcdsaAuthorization. The seize function does nothing except of emitting an event about detecting beta staker misbehavior.

To be done

Deployment script

We need to capture all existing beta stakers along with their current authorizations and initialize the Allowlist contract. We can do it by either replicating the existing weights or giving them all the same weight.

Integrate with WalletRegistry and tests

There are two approaches to achieve it. The first one is to get rid of all references to TokenStaking from tests and update them to work with Allowlist. Another approach is to let them work with TokenStaking but introduce another integration test for those two contracts. In this option, we could use in WalletRegistry something like:

    modifier onlyStakingContract() {
        address _allowlist = address(allowlist);
        require(
            // If the allowlist is set, accept calls only from the allowlist.
            // This is post-TIP-98 scenario. If the allowlist is not set, accept
            // calls only from the staking contract. This is pre-TIP-98 scenario.
            (_allowlist != address(0) && msg.sender == _allowlist) ||
                (_allowlist == address(0) && msg.sender == address(staking)),
            "Caller is not the staking contract"
        );
        _;
    }

    /// @notice Initializes V2 version of the WalletRegistry operating with the
    ///         Allowlist contract, as a result of TIP-098 and TIP-100 governance
    ///         decisions.
    function initializeV2(address _allowlist) external reinitializer(2) {
        allowlist = Allowlist(_allowlist);
    }

    /// @dev Provides the expected IStaking reference. If the allowlist is set,
    ///      it acts as the staking contract. If it is not set, the TokenStaking
    ///      acts as the staking contract.
    function _staking() internal returns (IStaking) {
        if (address(allowlist) != address(0)) {
            return IStaking(allowlist);
        }

        return staking;
    }

Note that the WalletRegistry is close to the maximum allowed contract size and - surprise! - adding the logic above makes it exceed the allowed size. This could potentially be alleviated by removing some of the functionality. For example, in the challengeDkgResult function we have a try catch as well as a call to dkg.requireChallengeExtraGas(). This could potentially be eliminated as a no-op seize in Allowlist is guaranteed to always succeed. Also, post EIP-7702, the require(msg.sender == tx.origin, "Not EOA") check is no longer guaranteed to work as expected.

pdyraga added 3 commits March 5, 2025 08:48
The current project configuration supports at most node 18.x so setting
lts/hydrogen as the version to be used.
The project is on OpenZeppelin 4 and the latest version currently
available is 4.9.6. This change will also allow us to use
Ownable2StepUpgradeable that was not available in OpenZeppelin 4.6. The
upgrade required increasing the version of
@openzeppelin/hardhat-upgrades plugin given the bug in the previously
used 1.20.0 version not resolving the correct version of dependencies
used (OZ contracts vs contracts-upgradeable) and in our case complaining
about the delegatecall used in the OZ's AddressUpgradeable code. The bug
was fixed in 1.20.4 version of the plugin. This also required updates in
the deployment tests as some functions are no longer available in the
upgraded version.

https://forum.openzeppelin.com/t/interacting-with-uups-upgradeable-contracts-in-test-throwing-contract-is-not-upgrade-safe-use-of-delegatecall-is-not-allowed/32743/5
The allowlist contract replaces the Threshold `TokenStaking` contract and
is as an outcome of TIP-092 and TIP-100 governance decisions. Staking
tokens is no longer required to operate nodes. Beta stakers are selected
by the DAO and operate the network based on the allowlist maintained by
the DAO. The contract will be integrated with the `WalletRegistry` and
replace calls to `TokenStaking`.

I have been experimenting with various approaches, and the most extreme
one was to remove most of the `EcdsaAuthorization` logic as well as all
`TokenStaking.seize` calls. This would have cascading effects on tBTC
Bridge contracts as they rely on `WalletRegistry.seize`. That would also
require implementing weight decrease delays in the `Allowlist,` so
essentially doing work that is already done in `WalletRegistry`.
Considering the pros and cons, I decided on the least invasive option.
The `WalletRegistry` still thinks in terms of stake authorization, but
everything is based on the staking provider's weight as set in the
`Allowlist`, and weight decrease delays are enforced by the existing
mechanism in `EcdsaAuthorization`. The `seize` function does nothing
except of emitting an event about detecting beta staker misbehavior.

This code is still a draft. Has to be integrated and covered with proper
tests!
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.

1 participant