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

EVM: DST20 v2 burn of sent amount #2911

Merged
merged 2 commits into from
May 7, 2024
Merged

EVM: DST20 v2 burn of sent amount #2911

merged 2 commits into from
May 7, 2024

Conversation

Jouzo
Copy link
Collaborator

@Jouzo Jouzo commented May 6, 2024

Summary

Screenshot 2024-05-06 at 11 30 03 Screenshot 2024-05-06 at 11 30 13

Implications

  • Consensus
    • changi rollback

@Bushstar
Copy link
Member

Bushstar commented May 6, 2024

If we are no longer going to zero the old token amount then we should not deduct that amount from the economy Gov vars as it still exists inside EVM on the burn address.

@Jouzo
Copy link
Collaborator Author

Jouzo commented May 6, 2024

We do zero it through calling the internal _burn method which handles deducting the total supply. You can see on the first screenshot that the META/v1 total supply is now correctly zeroed out

@Jouzo
Copy link
Collaborator Author

Jouzo commented May 6, 2024

function _burn(address account, uint256 amount) internal virtual {
        require(account != address(0), "ERC20: burn from the zero address");

        _beforeTokenTransfer(account, address(0), amount);

        uint256 accountBalance = _balances[account];
        require(accountBalance >= amount, "ERC20: burn amount exceeds balance");
        unchecked {
            _balances[account] = accountBalance - amount;
            // Overflow not possible: amount <= accountBalance <= totalSupply.
            _totalSupply -= amount;
        }

        emit Transfer(account, address(0), amount);

        _afterTokenTransfer(account, address(0), amount);
    }

Our implementation consider the burn as a reduction of totalSupply.
It's our appreciation how sending to the burn address should be counted for the gov vars economy. I would say that deducting it sounds right since it won't be accessible.

require(from != address(0), "ERC20: transfer from the zero address"); checks are scatted in the other methods and prevent the burn address from sending any amount

@Bushstar
Copy link
Member

Bushstar commented May 6, 2024

That sounds good to me. We can leave the economy Gov vars as they are.

@prasannavl prasannavl merged commit 6ec7477 into master May 7, 2024
22 of 26 checks passed
@prasannavl prasannavl deleted the fix/dst20_v2_burn branch May 7, 2024 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EVM: token transfers on stock split (upgradeToken) misses inflow
3 participants