Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposed modification of getPowerUpInfo in the powerUps contract #23

Open
Austin-Williams opened this issue Apr 19, 2019 · 1 comment
Open
Assignees
Labels
❗low Low priority issue

Comments

@Austin-Williams
Copy link
Collaborator

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 Austin-Williams added the ❗low Low priority issue label Apr 19, 2019
@sameepsi
Copy link
Collaborator

sameepsi commented May 1, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
❗low Low priority issue
Projects
None yet
Development

No branches or pull requests

2 participants