diff --git a/packages/ethereum-contracts/CHANGELOG.md b/packages/ethereum-contracts/CHANGELOG.md index 12d64aabed..ddef4c7f1d 100644 --- a/packages/ethereum-contracts/CHANGELOG.md +++ b/packages/ethereum-contracts/CHANGELOG.md @@ -20,6 +20,13 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - `SuperTokenV1Library.distributeFlow`: return `actualFlowRate` instead of a bool - `SuperTokenV1Library.distribute`: return `actualAmount` instead of a bool +# Breaking +- CFASuperAppBase: `onFlowDeleted` from now on only handles events related to incoming flows, while for events triggered by outgoing flows `onOutFlowDeleted` is invoked. + This is safer because the latter case is in many cases unexpected and may thus not be handled correctly, potentially leading to state corruption or SuperApp jailing. + The change is breaking because of a signature change in `onFlowDeleted`. The removal of the now unnecessary `receiver` argument also makes sure + that this change can't without notice break implementations which correctly handled the corner case of an outgoing flow with the previous implementation of CFASuperAppBase. + Many applications may not want/need to handle the case of outgoing flows being deleted, thus don't need to override the newly added `onOutFlowDeleted`. + ## [v1.12.0] ### Added diff --git a/packages/ethereum-contracts/contracts/apps/CFASuperAppBase.sol b/packages/ethereum-contracts/contracts/apps/CFASuperAppBase.sol index ec91b3e5e4..2e59a1e1ae 100644 --- a/packages/ethereum-contracts/contracts/apps/CFASuperAppBase.sol +++ b/packages/ethereum-contracts/contracts/apps/CFASuperAppBase.sol @@ -8,20 +8,27 @@ import { SuperTokenV1Library } from "./SuperTokenV1Library.sol"; * @title abstract base contract for SuperApps using CFA callbacks * @author Superfluid * @dev This contract provides a more convenient API for implementing CFA callbacks. - * It allows to write more concise and readable SuperApps when the full flexibility - * of the low-level agreement callbacks isn't needed. - * The API is tailored for the most common use cases, with the "beforeX" and "afterX" callbacks being + * It allows to write more concise and readable SuperApps. + * The API is tailored for common use cases, with the "beforeX" and "afterX" callbacks being * abstrated into a single "onX" callback for create|update|delete flows. - * For use cases requiring more flexibility (specifically if more data needs to be provided by the before callbacks) - * it's recommended to implement the low-level callbacks directly instead of using this base contract. + * If the previous state provided by this API (`previousFlowRate` and `lastUpdated`) is not sufficient for you use case, + * you should implement the more generic low-level API of `ISuperApp` instead of using this base contract. */ abstract contract CFASuperAppBase is ISuperApp { using SuperTokenV1Library for ISuperToken; + /// ================================================================================= + /// CONSTANTS & IMMUTABLES + /// ================================================================================= + bytes32 public constant CFAV1_TYPE = keccak256("org.superfluid-finance.agreements.ConstantFlowAgreement.v1"); ISuperfluid public immutable HOST; + /// ================================================================================= + /// ERRORS + /// ================================================================================= + /// @dev Thrown when the callback caller is not the host. error UnauthorizedHost(); @@ -31,6 +38,10 @@ abstract contract CFASuperAppBase is ISuperApp { /// @dev Thrown when SuperTokens not accepted by the SuperApp are streamed to it error NotAcceptedSuperToken(); + // ================================================================================= + // SETUP + // ================================================================================= + /** * @dev Creates the contract tied to the provided Superfluid host * @param host_ the Superfluid host the SuperApp belongs to @@ -49,7 +60,7 @@ abstract contract CFASuperAppBase is ISuperApp { * * Note: if the App self-registers on a network with permissioned SuperApp registration, * self-registration can be used only if the tx.origin (EOA) is whitelisted as deployer. - * If a whitelisted factory is used, it needs to call `host.registerApp()` itself. + * If instead a whitelisted factory is used, the factory needs to call `host.registerApp(address app)`. * For more details, see https://github.com/superfluid-finance/protocol-monorepo/wiki/Super-App-White-listing-Guide */ function selfRegister( @@ -72,7 +83,9 @@ abstract contract CFASuperAppBase is ISuperApp { bool activateOnUpdated, bool activateOnDeleted ) public pure returns (uint256 configWord) { + // since only 1 level is allowed by the protocol, we can hardcode APP_LEVEL_FINAL configWord = SuperAppDefinitions.APP_LEVEL_FINAL + // there's no information we want to carry over for create | SuperAppDefinitions.BEFORE_AGREEMENT_CREATED_NOOP; if (!activateOnCreated) { configWord |= SuperAppDefinitions.AFTER_AGREEMENT_CREATED_NOOP; @@ -96,10 +109,9 @@ abstract contract CFASuperAppBase is ISuperApp { return true; } - - // --------------------------------------------------------------------------------------------- - // CFA specific convenience callbacks - // to be overridden and implemented by inheriting SuperApps + // ================================================================================= + // CFA SPECIFIC CALLBACKS - TO BE OVERRIDDEN BY INHERITING SUPERAPPS + // ================================================================================= /// @dev override if the SuperApp shall have custom logic invoked when a new flow /// to it is created. @@ -130,6 +142,24 @@ abstract contract CFASuperAppBase is ISuperApp { function onFlowDeleted( ISuperToken /*superToken*/, address /*sender*/, + int96 /*previousFlowRate*/, + uint256 /*lastUpdated*/, + bytes calldata ctx + ) internal virtual returns (bytes memory /*newCtx*/) { + return ctx; + } + + /// @dev override if the SuperApp shall have custom logic invoked when an outgoing flow + /// is deleted by the receiver (it's not triggered when deleted by the SuperApp itself). + /// A possible implementation is to make outflows "sticky" by simply reopening it. + /// Like onFlowDeleted, this method is NOT allowed to revert. + /// It's safe to not override this method if the SuperApp doesn't have outgoing flows, + /// or if it doesn't want/need to know if an outgoing flow is deleted by its receiver. + /// Note: In theory this hook could also be triggered by a liquidation, but this would imply + /// that the SuperApp is insolvent, and would thus be jailed already. + /// Thus in practice this is triggered only when a receiver of an outgoing flow deletes that flow. + function onOutflowDeleted( + ISuperToken /*superToken*/, address /*receiver*/, int96 /*previousFlowRate*/, uint256 /*lastUpdated*/, @@ -138,12 +168,16 @@ abstract contract CFASuperAppBase is ISuperApp { return ctx; } + // ================================================================================= + // INTERNAL IMPLEMENTATION + // ================================================================================= - // --------------------------------------------------------------------------------------------- - // Low-level callbacks - // Shall NOT be overriden by SuperApps when inheriting from this contract. - // The before-callbacks are implemented to forward data (flowrate, timestamp), - // the after-callbacks invoke the CFA specific specific convenience callbacks. + // The following methods SHALL NOT BE OVERRIDDEN by SuperApps inheriting from this contract. + // If more fine grained control than provided by the onX callbacks is needed, + // you should implement the more generic low-level API of `ISuperApp` instead of using this base contract. + + // The before-callbacks are implemented to relay data (flowrate, timestamp) to the after-callbacks. + // The after-callbacks invoke the more convenient onX callbacks. // CREATED callback @@ -167,7 +201,7 @@ abstract contract CFASuperAppBase is ISuperApp { bytes calldata ctx ) external override returns (bytes memory newCtx) { if (msg.sender != address(HOST)) revert UnauthorizedHost(); - if (!isAcceptedAgreement(agreementClass)) return ctx; + if (!_isAcceptedAgreement(agreementClass)) return ctx; if (!isAcceptedSuperToken(superToken)) revert NotAcceptedSuperToken(); (address sender, ) = abi.decode(agreementData, (address, address)); @@ -190,7 +224,7 @@ abstract contract CFASuperAppBase is ISuperApp { bytes calldata /*ctx*/ ) external view override returns (bytes memory /*beforeData*/) { if (msg.sender != address(HOST)) revert UnauthorizedHost(); - if (!isAcceptedAgreement(agreementClass)) return "0x"; + if (!_isAcceptedAgreement(agreementClass)) return "0x"; if (!isAcceptedSuperToken(superToken)) revert NotAcceptedSuperToken(); (address sender, ) = abi.decode(agreementData, (address, address)); @@ -211,7 +245,7 @@ abstract contract CFASuperAppBase is ISuperApp { bytes calldata ctx ) external override returns (bytes memory newCtx) { if (msg.sender != address(HOST)) revert UnauthorizedHost(); - if (!isAcceptedAgreement(agreementClass)) return ctx; + if (!_isAcceptedAgreement(agreementClass)) return ctx; if (!isAcceptedSuperToken(superToken)) revert NotAcceptedSuperToken(); (address sender, ) = abi.decode(agreementData, (address, address)); @@ -238,7 +272,7 @@ abstract contract CFASuperAppBase is ISuperApp { ) external view override returns (bytes memory /*beforeData*/) { // we're not allowed to revert in this callback, thus just return empty beforeData on failing checks if (msg.sender != address(HOST) - || !isAcceptedAgreement(agreementClass) + || !_isAcceptedAgreement(agreementClass) || !isAcceptedSuperToken(superToken)) { return "0x"; @@ -263,7 +297,7 @@ abstract contract CFASuperAppBase is ISuperApp { ) external override returns (bytes memory newCtx) { // we're not allowed to revert in this callback, thus just return ctx on failing checks if (msg.sender != address(HOST) - || !isAcceptedAgreement(agreementClass) + || !_isAcceptedAgreement(agreementClass) || !isAcceptedSuperToken(superToken)) { return ctx; @@ -272,15 +306,25 @@ abstract contract CFASuperAppBase is ISuperApp { (address sender, address receiver) = abi.decode(agreementData, (address, address)); (uint256 lastUpdated, int96 previousFlowRate) = abi.decode(cbdata, (uint256, int96)); - return - onFlowDeleted( - superToken, - sender, - receiver, - previousFlowRate, - lastUpdated, - ctx - ); + if (receiver == address(this)) { + return + onFlowDeleted( + superToken, + sender, + previousFlowRate, + lastUpdated, + ctx + ); + } else { + return + onOutflowDeleted( + superToken, + receiver, + previousFlowRate, + lastUpdated, + ctx + ); + } } @@ -292,7 +336,7 @@ abstract contract CFASuperAppBase is ISuperApp { * This function can be overridden with custom logic and to revert if desired * Current implementation expects ConstantFlowAgreement */ - function isAcceptedAgreement(address agreementClass) internal view virtual returns (bool) { + function _isAcceptedAgreement(address agreementClass) internal view returns (bool) { return agreementClass == address(HOST.getAgreementClass(CFAV1_TYPE)); } } diff --git a/packages/ethereum-contracts/contracts/apps/SuperAppBase.sol b/packages/ethereum-contracts/contracts/apps/SuperAppBase.sol index cd6c642daa..7c5a78face 100644 --- a/packages/ethereum-contracts/contracts/apps/SuperAppBase.sol +++ b/packages/ethereum-contracts/contracts/apps/SuperAppBase.sol @@ -5,6 +5,11 @@ pragma solidity >= 0.8.11; // solhint-disable-next-line no-global-import import "../interfaces/superfluid/ISuperfluid.sol"; +/** + * @title [DEPRECATED] Base contract which provides a reverting implementation of all ISuperApp methods. + * @author Superfluid + * @custom:deprecated Use an agreement specific base contract (e.g. `CFASuperAppBase`) or implement `ISuperApp`. + */ abstract contract SuperAppBase is ISuperApp { function beforeAgreementCreated( diff --git a/packages/ethereum-contracts/test/foundry/apps/CFASuperAppBase.t.sol b/packages/ethereum-contracts/test/foundry/apps/CFASuperAppBase.t.sol index 13d0cda4f3..5b44f97a66 100644 --- a/packages/ethereum-contracts/test/foundry/apps/CFASuperAppBase.t.sol +++ b/packages/ethereum-contracts/test/foundry/apps/CFASuperAppBase.t.sol @@ -194,7 +194,7 @@ contract CFASuperAppBaseTest is FoundrySuperfluidTester { vm.stopPrank(); } - // test delete flow + // test delete flow to superApp (incoming flow) function testDeleteFlowToSuperApp(int96 flowRate) public { flowRate = int96(bound(flowRate, 1, int96(uint96(type(uint32).max)))); vm.startPrank(alice); @@ -207,13 +207,27 @@ contract CFASuperAppBaseTest is FoundrySuperfluidTester { superToken.deleteFlow(alice, superAppAddress); assertEq(superToken.getFlowRate(alice, superAppAddress), 0, "SuperAppBase: deleteFlow2 | flowRate incorrect"); assertEq(superApp.afterSenderHolder(), alice, "SuperAppBase: deleteFlow2 | afterSenderHolder incorrect"); - assertEq( - superApp.afterReceiverHolder(), superAppAddress, "SuperAppBase: deleteFlow2 | afterReceiverHolder incorrect" - ); assertEq(superApp.oldFlowRateHolder(), flowRate, "SuperAppBase: deleteFlow2 | oldFlowRateHolder incorrect"); vm.stopPrank(); } + // test delete flow from superApp + function testDeleteFlowFromSuperApp(int96 flowRate) public { + flowRate = int96(bound(flowRate, 1, int96(uint96(type(uint32).max)))); + + vm.startPrank(alice); + // fund the superApp and start a stream from it to alice + superToken.transfer(superAppAddress, 1e18); + superApp.startStream(superToken, alice, flowRate); + + // let alice delete the flow, triggering the onOutFlowDeleted callback + superToken.deleteFlow(superAppAddress, alice); + assertEq(superApp.lastUpdateHolder(), block.timestamp, "SuperAppBase: deleteFlow | lastUpdateHolder incorrect"); + assertEq(superApp.oldFlowRateHolder(), flowRate, "SuperAppBase: deleteFlow | oldFlowRateHolder incorrect"); + assertEq(superApp.afterReceiverHolder(), alice, "SuperAppBase: deleteFlow | afterReceiverHolder incorrect"); + vm.stopPrank(); + } + function testMockBeforeAgreementCreated() public { vm.startPrank(alice); bytes memory data = superApp.beforeAgreementCreated( diff --git a/packages/ethereum-contracts/test/foundry/apps/CFASuperAppBaseTester.t.sol b/packages/ethereum-contracts/test/foundry/apps/CFASuperAppBaseTester.t.sol index 48fbb81d27..9acbd9da1e 100644 --- a/packages/ethereum-contracts/test/foundry/apps/CFASuperAppBaseTester.t.sol +++ b/packages/ethereum-contracts/test/foundry/apps/CFASuperAppBaseTester.t.sol @@ -75,7 +75,6 @@ contract CFASuperAppBaseTester is CFASuperAppBase { function onFlowDeleted( ISuperToken, /*superToken*/ address sender, - address receiver, int96 previousFlowRate, uint256 lastUpdated, bytes calldata ctx @@ -83,6 +82,18 @@ contract CFASuperAppBaseTester is CFASuperAppBase { lastUpdateHolder = lastUpdated; oldFlowRateHolder = previousFlowRate; afterSenderHolder = sender; + return ctx; + } + + function onOutflowDeleted( + ISuperToken, /*superToken*/ + address receiver, + int96 previousFlowRate, + uint256 lastUpdated, + bytes calldata ctx + ) internal override returns (bytes memory newCtx) { + lastUpdateHolder = lastUpdated; + oldFlowRateHolder = previousFlowRate; afterReceiverHolder = receiver; return ctx; } diff --git a/packages/ethereum-contracts/test/foundry/apps/SuperAppTester/FlowSplitter.sol b/packages/ethereum-contracts/test/foundry/apps/SuperAppTester/FlowSplitter.sol index 808b9f34bb..16d50f9538 100644 --- a/packages/ethereum-contracts/test/foundry/apps/SuperAppTester/FlowSplitter.sol +++ b/packages/ethereum-contracts/test/foundry/apps/SuperAppTester/FlowSplitter.sol @@ -132,7 +132,6 @@ contract FlowSplitter is CFASuperAppBase { function onFlowDeleted( ISuperToken superToken, address, /*sender*/ - address receiver, int96 previousFlowRate, uint256, /*lastUpdated*/ bytes calldata ctx @@ -145,11 +144,6 @@ contract FlowSplitter is CFASuperAppBase { + acceptedSuperToken.getFlowRate(address(this), sideReceiver) ) - previousFlowRate; - // handle "rogue recipients" with sticky stream - see readme - if (receiver == mainReceiver || receiver == sideReceiver) { - newCtx = superToken.createFlowWithCtx(receiver, previousFlowRate, newCtx); - } - // if there is no more inflow, outflows should be deleted if (remainingInflow <= 0) { newCtx = superToken.deleteFlowWithCtx(address(this), mainReceiver, newCtx); @@ -165,4 +159,19 @@ contract FlowSplitter is CFASuperAppBase { newCtx = superToken.updateFlowWithCtx(sideReceiver, (remainingInflow * sideReceiverPortion) / 1000, newCtx); } } + + function onOutflowDeleted( + ISuperToken superToken, + address receiver, + int96 previousFlowRate, + uint256, /*lastUpdated*/ + bytes calldata ctx + ) internal override returns (bytes memory newCtx) { + newCtx = ctx; + + // handle "rogue recipients" with sticky stream - see readme + if (receiver == mainReceiver || receiver == sideReceiver) { + newCtx = superToken.createFlowWithCtx(receiver, previousFlowRate, newCtx); + } + } }