storyprotocol / protocol-periphery-v1

Periphery contract for story protocol, mainnet repo
MIT License
28 stars 20 forks source link

SPG - Derivative Function / Commercial License Issue #21

Closed bpolania closed 2 months ago

bpolania commented 2 months ago

Description and context

In SPG, after minting and registering an IP asset with PIL terms, registering a derivate IP asset fails and throws the error ERC20InsufficientAllowance(address,uint256,uint256), https://openchain.xyz/signatures?query=0xfb8f41b2

The primary problem appears to be that msg.sender changes from an externally owned account (EOA) to the SPG address when SPG calls ILicensingModule.registerDerivative.

Steps to reproduce

Follow this guide to reproduce https://www.notion.so/storyprotocol/SPG-register-derivate-IP-with-commercial-license-term-fail-33faabd2cca344ea9a7f3b7dff679ee3

Experienced behavior

Transaction fails with ERC20InsufficientAllowance error

Expected behavior

It should be possible to register derivative IPs with commercial licenses

Proposed Solutions

57Blocks team suggests using delegate calls to call the registerDerivative function so the msg.sender is preserved in subsequent sub-calls:

(bool success, ) = address(LICENSING_MODULE).delegatecall(
    abi.encodeWithSelector(
        ILicensingModule.registerDerivative.selector,
        ipId,
        derivData.parentIpIds,
        derivData.licenseTermsIds,
        derivData.licenseTemplate,
        derivData.royaltyContext
    )
);
if (!success) revert Errors.SPG__RegisterDerivativeFailed();
kingster-will commented 2 months ago

The ERC20InsufficientAllowance error occurs when the child IP owner hasn't approved the RoyaltyPolicy to transfer ERC20 tokens from their wallet to the parent IP owners' wallets. We cannot rely solely on delegate calls for all cases. For instance, the SDK's mintLicenseTokens function, which directly calls the protocol core, also requires ERC20 approval from the user.

Reference:

We should adopt a unified approach to handle scenarios requiring ERC20 approval, such as the following example from the licensing integration test:

erc20.approve(address(royaltyPolicyLAP), 100);

Reference:

kingster-will commented 2 months ago

@sebsadface Please looked at the issue. I think to fix the issue we need to make change into SPG that: in the function registerIpAndMakeDerivative do following:

  1. Transfer currency token(ERC20) from caller to SPG contract
  2. SPG set approval of ERC20 to royalty LAP policy.