hypercerts-org / hypercerts

Hypercerts are a tool to build scalable retrospective reward systems for impact.
https://hypercerts.org/
Other
93 stars 29 forks source link

[EVM][FE] Burn batch #732

Closed bitbeckers closed 1 year ago

bitbeckers commented 1 year ago

Describe the feature you'd like to request

When a holder wants to burn multiple fractions, we don't provide an appropriate method. The underlying erc1155 contract allows for batch burning -and exposes the method- but that's skipping some hypercert specific steps like emitting ValueTransfer events (fraction units)

Describe the solution you'd like

Describe alternatives you've considered

Using the underlying method. But we'd lose some state changes and events specific to Hypercerts

bitbeckers commented 1 year ago

@ryscheng consideration: Following the rest of the contract patterns batch burning tokens would like something like this:

  /// @dev Batch burn the token at `_tokenIsD` owned by `_account`
    /// @dev Not allowed to burn base type.
    /// @dev `_tokenID` must hold all value declared at base type
    function _batchBurnToken(address _account, uint256[] memory _tokenIDs) internal {
        if (_account != _msgSender() && !isApprovedForAll(_account, _msgSender())) revert Errors.NotApprovedOrOwner();

        // ERC115 requires values
        uint256[] memory claimIDs = new uint256[](len);
        address[] memory toTokens = new uint256[](len);
        uint256[] memory claimUnits = new uint256[](len);
        uint256[] memory values = new uint256[](len);

        for (uint256 i; i < len; i++) {
            uint256 _tokenId = _tokenIDs[i];
            uint256 value = tokenValues[_tokenID];

            delete tokenValues[_tokenID];

            claimIDs[i] = getBaseType(_tokenId);
            claimUnits[i] = value;
            values[i] = 1;
        }

        _burnBatch(_account, _tokenIDs, values);
        emit BatchValueTransfer(claimIDs, _tokenIDs, toTokens, claimUnits);
    }

Alternatively, we can expect the user to provide the correct input and skip these in-tx data fetching. There are inherent checks whether tokens exists and are owned, so this might be more efficient?

bitbeckers commented 1 year ago

@ryscheng we have two pretty identical functions and I think it's prettier if we follow the ERC1155 standard implementation. The 'burnFraction' concept is probably an artifact?

What do you think about removing the top two methods and matching the ERC1155 standard?

/// @notice Burn a claimtoken
    /// @dev see {IHypercertToken}
    function burnFraction(address _account, uint256 _tokenID) external whenNotPaused {
        _burnToken(_account, _tokenID);
    }

    /// @notice Burn a claimtoken
    /// @dev see {IHypercertToken}
    function batchBurnFraction(address _account, uint256[] memory _tokenIDs) external whenNotPaused {
        _batchBurnToken(_account, _tokenID);
    }

    /// @notice Burn a claimtoken; override is needed to update values
    /// @dev see {ERC1155Burnable}
    function burn(address account, uint256[] memory ids, uint256[] memory values) public override whenNotPaused {
        _burnToken(account, id);
    }

    /// @notice Batch burn claimtokens; override is needed to update values
    /// @dev see {ERC1155Burnable}
    function batchBurn(address account, uint256[] calldata ids, uint256[] calldata values) public override whenNotPaused {
        _batchBurnToken(account, ids);
    }
ryscheng commented 1 year ago

I'm cool with moving closer to the ERC1155 standard. I don't recall when these were added...

I'm actually a little confused what is the implication of burning a claim token? Let's say there's Alice minted the hypercert and gave Bob 50%. So Alice owns the claim token and fraction A, and Bob owns fraction B.

What happens when Alice burns the claim token?

bitbeckers commented 1 year ago

We delelte the value (i.e. units) and burn the 1155 fractionToken.

The total units remains the same, so Bob still owns 50% of the Claim

ryscheng commented 1 year ago

Bob owns 50% of a claim, where the claim token no longer exists?

I guess that's reasonable.... in the UI is it worth showing that to be a different state? Maybe asking minters to burn claim tokens is a better way of marking duplicates?

bitbeckers commented 1 year ago

Or sorry I misread. No the claim token isn't owned by anyone, only fractions.

For example: Claim X is token 10 Alice owns 50% on token 11 Bob owns 50% on token 12 Alice can only burn 11

Claim y is token 20 Alice owns 50% on token 21 Alice owns 50% on token 22 Alice can burn 21 and 22

ryscheng commented 1 year ago

Oh ya, that was what I thought, but I got confused in the code snippet above on the difference between burn and burnFraction. In the comments it talks about claimtokens, which is a bit confusing terminology IMO

bitbeckers commented 1 year ago

To roll out for testnet Plasmic only: https://docs.plasmic.app/learn/multiple-environments/#managing-your-published-designs-with-tags

holkexyz commented 1 year ago

@bitbeckers so this is implemented in the contracts, but not in the frontend at the moment, correct? If so, this is fine for now and we don't need to build the frontend currently, i.e. this would be finished and the frontend can go into the backlog

bitbeckers commented 1 year ago

Closing for now based on @holkeb comment. It is available in the contract on Goerli, not on Optimism, and will also be available on other chains we roll out to.