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

USDC v2.2 #357

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

USDC v2.2 #357

wants to merge 8 commits into from

Conversation

alexkroeger
Copy link

@alexkroeger alexkroeger commented Jul 14, 2022

This PR is an alternative USDC v3 implementation (a minor version bump instead--v2.2). Unlike #338, it does not change any blacklist functionality of USDC, but accomplishes similar gas savings by stuffing the blacklist boolean into the high bit of a user's balance.

Check out this post on mirror.xyz for more context about why it's worth trying to save on gas for checking the blacklist.

I estimate that with these changes, USDC users would have spent ~$700K less on gas in June 2022 alone (see https://dune.com/queries/1074893).

Tasks:

  • Modify blacklist by storing the boolean in the high bit of the user's balance
  • Implement the "infinite allowance" hack for bonus gas savings (see https://twitter.com/philipliao_/status/1479860204577693700?s=20 for context)
  • Ensure all legacy tests are still working with the proposed changes
  • Write tests for infinite allowance hack
  • Add the ability to blacklist an array of accounts to initializeV3. This will allow existing blacklisted accounts to be re-blacklisted using the new method of changing the high bit in the account's balance.
  • Write tests for initializeV3.
  • Identify blacklisted accounts to feed into the upgrade contract for ETH L1. See:
  • Implement V3Upgrade contract
  • Write tests for V3Upgrade contract
  • Write truffle migrations for V3 implementation, V3Upgrade contract

Sorry, something went wrong.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.
…d script to find all currently blacklisted accounts

Unverified

This user has not yet uploaded their public signing key.
…3Upgrader
/**
* @notice Initialize v3
*/
function initializeV3(address[] calldata accountsToBlacklist) external {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any idea on approximately how many addresses we can include here? The blacklist grows somewhat erratically so wondering how far we have to go before we would have to come up with another way to migrate the blacklisted addresses.

Copy link
Author

@alexkroeger alexkroeger Jul 28, 2022

Choose a reason for hiding this comment

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

That's a good callout, I'll think about what the constraints are there (I think it would just be a tx gas constraint, e.g. how many blacklist calls you do within the gas limit of a transaction).

There are currently 41 addresses on the ETH L1 blacklist (see https://dune.com/queries/1057941), which I think should be well within the constraint, but definitely worth knowing what the upper bound is.

Copy link
Author

Choose a reason for hiding this comment

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

Thought about this some more--the gas footprint of each blacklist is mainly from the storage write, which costs 20K gas at worst (if the blacklisted user doesn't have a balance, only 2900 if there is a balance already there) for each blacklist call.

The size of Ethereum transactions is limited by block size.

Blocks can be bigger but 15M gas is the target block size. At 15M one could blacklist ~750 addresses in a single transaction assuming the worst case gas cost per address (but also assuming no other operations in the transaction). Taking into account the other checks performed in the upgrade transaction, let's call this ~700 address cap.

This should be more than adequate headspace given that USDC currently has 41 addrs on the blacklist.

image

Copy link
Contributor

Choose a reason for hiding this comment

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

well, now up to 81 addresses, but yeah still plenty of headroom.

contracts/v3/FiatTokenV3.sol Outdated Show resolved Hide resolved
// additionally blacklist the contract address itself
_blacklist(address(this));

DOMAIN_SEPARATOR = EIP712.makeDomainSeparator(name, "3");
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would invalidate all previous signatures for the EIP2612/3009 features ?
not sure if anyone is using it, but this might lead to some disruption around the upgrade if they are.

Copy link
Author

@alexkroeger alexkroeger Jul 28, 2022

Choose a reason for hiding this comment

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

It would, which I don't think is necessary.

Now that I'm thinking about it, I'm not sure this even needs to be considered a major version bump 😅 . While it made sense to call the previous proposal v3, since this is effectively keeping all functionality the same and not changing the interface, I think it might be more apt to call it v2.2.

Copy link
Author

Choose a reason for hiding this comment

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

Went ahead and made this a minor version bump to 2.2, and kept DOMAIN_SEPARATOR unchanged.

49a3737


(uint256 fromBalance, bool isFromBlacklisted) = _getBalanceAndBlacklistStatus(from);

require(!isFromBlacklisted, "Blacklistable: account is blacklisted");
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume the blacklist checks were moved here from the modifiers for maximum gas saving?

Copy link
Author

Choose a reason for hiding this comment

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

That's right, by moving the blacklist checks here, you can read from the balance storage slot just once, grabbing both the blacklist boolean and the balance.

You'd still get gas savings from keeping all these checks as modifiers, since it would be warm SLOADs instead of cold SLOADs the second time the storage slots are read, but this is more gas efficient.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.
@alexkroeger alexkroeger changed the title USDC v3, alternative implementation USDC v2.2 Aug 8, 2022

Unverified

This user has not yet uploaded their public signing key.
@circlefin circlefin deleted a comment Aug 11, 2022
@circlefin circlefin deleted a comment Aug 11, 2022
// with a high balance
// Hardcoding the value here as opposed to setting it in storage since it's
// expected to be static and this avoids an SLOAD
require(newTotalSupply <= (uint256(1) << 255) - 1, "mint causes total supply to supply cap");
Copy link
Contributor

Choose a reason for hiding this comment

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

ah, nice, so avoid doing this in transfer since no one can ever have a balance > totalSupply. So only mint gets more expensive.

Unverified

This user has not yet uploaded their public signing key.
Copy link

@Dioliode Dioliode left a comment

Choose a reason for hiding this comment

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

DioX

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.

None yet

4 participants