smart-contracts icon indicating copy to clipboard operation
smart-contracts copied to clipboard

Proposed modification of getPowerUpInfo in the powerUps contract

Open Austin-Williams opened this issue 6 years ago • 1 comments

The getPowerUpInfo function in the powerUps contract returns an "empty" powerUp when id is out of range / does not exist. Should we instead have getPowerUpInfo revert when the passed id is out of range / does not exist?

Current code:

/**
    * @dev Returns info about a given PowerUp
    * @param id The index of the PowerUp in the powerUps array
    */
    function getPowerUpInfo(
        uint256 id
    )
        external
        view
        returns (
            string memory contentAddress,
            uint256 tokensBurned,
            uint256 lastTopupTime,
            bytes32 keyword
        )
    {
        if (powerUps.length > id) {

            PowerUp storage powerUp = powerUps[id];

            contentAddress = powerUp.contentAddress;
            tokensBurned = powerUp.tokensBurned;
            lastTopupTime = powerUp.lastTopupTime;
            keyword = powerUp.keyword;

        }

        return (contentAddress, tokensBurned, lastTopupTime, keyword);
    }

Proposed change:

/**
    * @dev Returns info about a given PowerUp
    * @param id The index of the PowerUp in the powerUps array
    */
    function getPowerUpInfo(
        uint256 id
    )
        external
        view
        returns (
            string memory contentAddress,
            uint256 tokensBurned,
            uint256 lastTopupTime,
            bytes32 keyword
        )
    {
        require(id < powerUps.length, "id is out of range");

        PowerUp storage powerUp = powerUps[id];

        contentAddress = powerUp.contentAddress;
        tokensBurned = powerUp.tokensBurned;
        lastTopupTime = powerUp.lastTopupTime;
        keyword = powerUp.keyword;

        return (contentAddress, tokensBurned, lastTopupTime, keyword);
    }

Austin-Williams avatar Apr 19 '19 17:04 Austin-Williams

Yes, we should do this, not a problem. I believe we already made this change in our no-token branch.

So once it gets merged into master we will have this in master as well.

sameepsi avatar May 01 '19 08:05 sameepsi