superfluid-finance/protocol-monorepo

[ETHEREUM-CONTRACTS] [DEVX] simplify SuperApp registration

d10r opened this issue · 3 comments

d10r commented

The registration key doesn't serve any purpose (anymore) after switching to an expire date based logic instead of one-time keys.
As a devx improvement, we could now simplify the logic to this:

  • registerApp(configWord) for registration through the constructor, invoked by a deployer.
  • registerApp(app, configWord) for registration through a factory contract.

Overloading isn't an issue in the Solidity API (it's a devx issue only with e.g. ethers.js).

registerAppWithKey and registerAppByFactory can be kept for a while marked as deprecated (for backwards compatibility) and later removed. But docs and examples shouldn't use them anymore.

Internally (in governance contract and scripts) we can either keep the logic as is (declaring e.g. the empty string as default key) or simplify the code. It could be simplified to just whitelist deployers, regardless of EOA or contract.
If we want to keep the distinction and associated constraints (EOA: call only from constructor), that could be achieved implicitly by checking if the msg.sender is an EOA or contract in registerApp.

https://github.com/superfluid-finance/protocol-monorepo/wiki/About-App-Registry updated accordingly:

About APP_RULE_REGISTRATION_ONLY_IN_CONSTRUCTOR:

    This is called the APP_RULE_REGISTRATION_ONLY_IN_CONSTRUCTOR rule. The intent for this rule is to prevent a contract from upgrading itself to Super app after the fact. Retroactively, APP_RULE_REGISTRATION_ONLY_IN_CONSTRUCTOR is more on the nanny-state rule side, since it tries to protect users from malfunctioning or malicious Super apps.

About app registration key:

    The design purpose of the key is to allow whitelisting process to have "finer control" over the same EOA.

--

After some discussion, the decision is to:

deprecate APP_RULE_REGISTRATION_ONLY_IN_CONSTRUCTOR nanny rule and registration key usage.

--

However, in the long run, the goal is still to depreciate app registration whitelisting process.

Waiting for the PR #1706 to be ready.

d10r commented

now behaves like this for existing authorizations:

  • registered factories shall keep working
  • existing registration keys shall keep working

new authorizations:

  • are created using the ops-script gov-authorize-app-deployer.js
  • deployer account can be EOA or contract
  • registerApp is used in all cases
    • if an app argument is NOT provided, it's a self-registration by a SuperApp. In this case the tx.origin (EOA) needs to be authorized
    • if an app argument is provided, the msg.sender needs to be authorized (may be a contract)

This covers all previous possibilities, but makes less assumptions. E.g. also allows registration of upgradable SuperApps without "abusing" a registration method not meant to be used that way.

All new authorizations are mapped to the gov config key org.superfluid-finance.superfluid.appWhiteListing.registrationKey, setting the key to "k1". That's not pretty, but we decided it's not worth the complication (e.g. breakage of existing authorizations) to change this again. Focus was on simplifying the public API and devx for SuperApp devs.

The expiration date is now set to (effectively) unlimited by default (to be more precise, to 2^64 - 1). It can still be set to any other value if the optional env var EXPIRATION_TS is set.