superfluid-finance / protocol-monorepo

Superfluid Protocol Monorepo: the specification, implementations, peripherals and development kits.
https://www.superfluid.finance
Other
874 stars 238 forks source link

[SUBGRAPH] superTokenLogicCreatedEvents missing recent events #1912

Open d10r opened 6 months ago

d10r commented 6 months ago

The script for updating Super Tokens queries superTokenLogicCreatedEvents in order to get a list of previous canonical logic contracts, see https://github.com/superfluid-finance/protocol-monorepo/blob/dev/packages/ethereum-contracts/ops-scripts/gov-upgrade-super-token-logic.js#L166

However this query returns incomplete data, it doesn't include recent deployments.

Example: On optimism-mainnet, the latest canonical logic contract is 0x78743a68d52c9D6CCF3fF4558f3af510592E3C2D, see factory.

The SG query

query MyQuery {
  superTokenLogicCreatedEvents(orderDirection: desc, orderBy: timestamp) {
    id
    tokenLogic
  }
}

returns 0x882703dc8239e2ba167e06ce1fcf654e17a0bd06 as the most recent logic contract, which was deployed a year ago.

Without having checked, I guess the breakage may be related to switching to Custom Errors or to the restructuring of the SuperTokenFactory (canonical logic now being deployed in a dedicated tx by the script).

d10r commented 5 days ago
network current canonical logic SG last logic SG last event timestamp
Ethereum 0xF0d7d1D47109bA426B9D8A3Cde1941327af1eea3 0xeb69ed9143d33d5fbad67f394456f212c65c1544 1679413211
Polygon 0xfD83982eE75892781242141Ee19E1B42428B8220 0x5ee041478e482ff756a783b8115b06bcecc0fc93 1679412431
Base 0xEB796bdb90fFA0f28255275e16936D25d3418603 N/A N/A
Base Sepolia 0x233a5Bfd65Da07AeB08F2082d2B5B270bc4eA804 N/A N/A
d10r commented 5 days ago

Reason for the missing events: when moving creation of the SuperToken logic out of the SuperTokenFactory (due to contract size limit), we moved emission of the event SuperTokenLogicCreated into the constructor of SuperTokenFactory where it's set as an immutable. Problem with this approach: the event is now emitted by the SuperTokenFactory logic contract, not by the proxy contract where the subgraph expects to see it.

d10r commented 5 days ago

recommendation: remove the event

rationale: The script gov-upgrade-super-token-logic.js triggering discovery of this bug is the only consumer of the event. Its recently added querying of this event was a small ergonomic improvement, not something really need. What we need is the previous canonical SuperToken logic, which is not difficult to get in other ways. E.g. when preparing gov actions for mainnets with the upgrade action not yet executed, it's even as simple as querying the current canonical ST logic. Regardless, we shouldn't leave it broken like this, with the subgraph pretending to have the events, but having only old ones. There's no good solution for fixing it in the subgraph, such that missed events were included. We could fix it for future events by moving the event emission statement somewhere else in the protocol, e.g. into updateCode. But in order to avoid duplicates when the canonical logic doesn't change between factory upgrades (unlikely, but possible), additional logic and to-be-tested code paths would be created. Unjustified bloat. Instead it's an opportunity to simplify by removing the event.

If we wanted to cover contract upgrades in the subgraph, a better and more generic approach would be to track the event CodeUpdated for all upgradable contracts. We're currently not doing that.