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

[ETHEREUM-CONTRACTS] [DEVX] simplify SuperApp registration #1660

Closed
d10r opened this issue Sep 5, 2023 · 3 comments · Fixed by #1706
Closed

[ETHEREUM-CONTRACTS] [DEVX] simplify SuperApp registration #1660

d10r opened this issue Sep 5, 2023 · 3 comments · Fixed by #1706
Assignees
Labels
Tag: TechDebt Technical debt that needs to be addressed Team: Protocol Protocol Core, Sentinel, Peripherals, Protocol Infrastructure Tools & DevOps

Comments

@d10r
Copy link
Collaborator

d10r commented Sep 5, 2023

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.

@d10r d10r added the Team: Protocol Protocol Core, Sentinel, Peripherals, Protocol Infrastructure Tools & DevOps label Sep 5, 2023
@d10r d10r self-assigned this Sep 5, 2023
@0xdavinchee 0xdavinchee assigned 0xdavinchee and unassigned d10r Sep 22, 2023
@hellwolf hellwolf added the Tag: TechDebt Technical debt that needs to be addressed label Sep 28, 2023
@0xdavinchee 0xdavinchee linked a pull request Sep 29, 2023 that will close this issue
@hellwolf
Copy link
Contributor

hellwolf commented Oct 3, 2023

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.

@hellwolf
Copy link
Contributor

hellwolf commented Oct 6, 2023

Waiting for the PR #1706 to be ready.

@d10r
Copy link
Collaborator Author

d10r commented Oct 8, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tag: TechDebt Technical debt that needs to be addressed Team: Protocol Protocol Core, Sentinel, Peripherals, Protocol Infrastructure Tools & DevOps
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants