gnosis / conditional-tokens-contracts

Smart contracts for conditional tokens.
GNU Lesser General Public License v3.0
163 stars 64 forks source link

Clearing storage of irrelevant values #34

Closed xavierlepretre closed 5 years ago

xavierlepretre commented 5 years ago

After a condition has been determined, by the oracle, and redeemed, by participants, there still remain non-0 balances in the ERC-1155. Namely, the worthless positions. This is keeping the Ethereum global state unnecessarily large, akin to pollution.

From a good citizenship point of view, it is desirable to reduce the global state.

Would you consider adding a function that can be called by anyone and that burns the positions that can indeed be burned? Essentially, such a function looks like the redeemPositions function, with:

What would be the incentive for the good Samaritan? Presumably, if the rest of the function is cheap enough, the SSTORE to 0 returns enough gas to encourage arbitrage.

The same, perhaps, could be done for the payout vector.

Example, devoid of unnecessary checks to keep gas cost low:

function clearPositions(IERC20 collateralToken, bytes32 parentCollectionId, bytes32 conditionId, uint[] calldata indexSets, address beneficiary) external {
    require(payoutDenominator[conditionId] > 0, "result for condition not received yet");
    uint outcomeSlotCount = payoutNumerators[conditionId].length;
    require(outcomeSlotCount > 0, "condition not prepared yet");

    bytes32 key;

    for (uint i = 0; i < indexSets.length; i++) {
        uint indexSet = indexSets[i];
        key = keccak256(abi.encodePacked(collateralToken, getCollectionId(parentCollectionId, conditionId, indexSet)));

        for (uint j = 0; j < outcomeSlotCount; j++) {
            require(indexSet & (1 << j) == 0 || payoutNumerators[conditionId][j] == 0);
        }

        uint payoutStake = balanceOf(beneficiary, uint(key));
        // Perhaps make a cheaper _burnToZero method that does = 0 instead of .sub(payoutStake).
        _burn(msg.sender, uint(key), payoutStake);
    }

    emit PayoutRedemption(beneficiary, collateralToken, parentCollectionId, conditionId, indexSets, 0);
}
cag commented 5 years ago

Hey yeah, that's an interesting thought. This isn't by any chance a final implementation though. Maybe we could even make the redeem function take a beneficiary instead of having it operate for the msg.sender, and that way, storage may be cleaned up regardless of whether or not an outcome collection is valueless.

I don't think it will be possible to clear out the payout vectors from the state though, but at least clearing out balances could help...

On incentivizing this behavior, it would probably make more sense for contract wallet which is able to batch multiple calls into a single transaction. A more expensive call to a different contract may be bundled with a gas refunded one on the PM system, and maybe a service which looks for these refund opportunities may be created so that this wallet potentially gets discounts on transactions for clearing out state on the PM system. Of course, as you've mentioned, redeem/something would have to be cheap enough to cause such a discount to be possible... I've not measured to see if this can be made the case yet though.

cag commented 5 years ago

We talked internally about this issue, and I believe the decision made was to skip doing this for the first release and wait to see how storage rent will be implemented. One of the issues is that we will be trying to finalize the design of this contract soon, and bringing this feature to production while making it is still secure is too much for us to handle right now. It might be good in a future release though.