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

add OptimismMintableFiatTokenV2_2 #1

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

add OptimismMintableFiatTokenV2_2 #1

wants to merge 21 commits into from

Conversation

alvrs
Copy link
Member

@alvrs alvrs commented Apr 24, 2024

Context:

For normal ERC20 tokens we could use Optimism's ERC20 factory, but for USDC we need to follow the Bridged USDC Standard in order to have the option to eventually migrate the contract to a native Circle issued USDC, instead of ending up with two USDC variants on the L2.

@alvrs alvrs marked this pull request as ready for review April 24, 2024 19:05
@alvrs alvrs marked this pull request as draft April 24, 2024 19:05
@alvrs alvrs requested review from ludns, holic, Kooshaba and yonadaaa April 24, 2024 20:58
@alvrs alvrs marked this pull request as ready for review April 24, 2024 20:58
@alvrs alvrs requested review from karooolis and dk1a April 24, 2024 21:11
// SPDX-License-Identifier: MIT
pragma solidity 0.6.12;

import { FiatTokenV1 } from "../v1/FiatTokenV1.sol";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused import, can remove

totalSupply_ = totalSupply_.sub(_amount);
_setBalance(_from, balance.sub(_amount));
emit Burn(_from, _amount);
emit Transfer(_from, address(0), _amount);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if emit Transfer makes sense here? We are not transferring tokens to address(0) as far as I can tell, just decreasing totalSupply and user's balance.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is taken from FiatTokenV1, only change is the _from param instead of using msg.sender:

/**
* @notice Allows a minter to burn some of its own tokens.
* @dev The caller must be a minter, must not be blacklisted, and the amount to burn
* should be less than or equal to the account's balance.
* @param _amount the amount of tokens to be burned.
*/
function burn(uint256 _amount)
external
whenNotPaused
onlyMinters
notBlacklisted(msg.sender)
{
uint256 balance = _balanceOf(msg.sender);
require(_amount > 0, "FiatToken: burn amount not greater than 0");
require(balance >= _amount, "FiatToken: burn amount exceeds balance");
totalSupply_ = totalSupply_.sub(_amount);
_setBalance(msg.sender, balance.sub(_amount));
emit Burn(msg.sender, _amount);
emit Transfer(msg.sender, address(0), _amount);
}

Copy link

@karooolis karooolis Aug 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the OpenZeppelin example though there is an _update function that increases the balance of address(0) - https://github.com/OpenZeppelin/openzeppelin-contracts/blob/e3786e63e6def6f3b71ce7b4b30906123bffe67c/contracts/token/ERC20/ERC20.sol#L206 so it makes sense to emit Transfer to address(0).

Although I see in Solmate's case the balance is also not increased. And also was not aware of such pattern. I suppose it's useful if you want to monitor for burned tokens, fair enough.

@alvrs alvrs changed the title add OptimismFiatTokenV2_2 add OptimismMintableFiatTokenV2_2 Aug 1, 2024
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.

4 participants