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

Burn using 0xdead to enable taking reserves on L2s #1058

Merged
merged 5 commits into from
Jan 17, 2024
Merged

Burn using 0xdead to enable taking reserves on L2s #1058

merged 5 commits into from
Jan 17, 2024

Conversation

EdNoepel
Copy link
Contributor

@EdNoepel EdNoepel commented Jan 16, 2024

Description

When taking reserve auctions, replace burn with a transfer to the 0xdead address.

Purpose

Discovered the L2-bridged bwAJNA tokens did not have a standard burn(uint256 amount) method. Base/Optimism/Polygon have a two-param burn, Arbitrum doesn't have one at all. Sending to 0x0 is prohibited by wormhole and gnosis, so using 0xdead instead.

Impact

Small decrease in gas cost is expected, since we're eliminating the call yet still transfer to another address, rather than transferring to pool before burning. Significant decrease in gas cost observed.

Pool contract size reduced by 103 bytes.

Unit test failure unreleated to these changes.

Tasks

  • Changes to protocol contracts are covered by unit tests executed by CI.
  • Protocol contract size limits have not been exceeded.
  • Gas consumption for impacted transactions have been compared with the target branch, and nontrivial changes cited in the Impact section above.
  • Scope labels have been assigned as appropriate.
  • Invariant tests have been manually executed as appropriate for the nature of the change.

@EdNoepel EdNoepel changed the title L2 burn Burn using 0xdead to enable taking reserves on L2s Jan 16, 2024
@@ -21,8 +24,5 @@ ETH_GAS=15000000
## Solidity Compiler Version ##
SOLC_VERSION=0.8.18

## AJNA token address for target chain ##
AJNA_TOKEN=0xaadebCF61AA7Da0573b524DE57c67aDa797D46c5
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This wasn't being used anywhere. Had it commented-out in my env file for many months.

Copy link
Contributor

@k1rill-fedoseev k1rill-fedoseev left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -433,9 +435,7 @@ abstract contract Pool is Clone, ReentrancyGuard, Multicall, IPool {
);

// burn required number of ajna tokens to take quote from reserves
IERC20(_getArgAddress(AJNA_ADDRESS)).safeTransferFrom(msg.sender, address(this), ajnaRequired);

IERC20Token(_getArgAddress(AJNA_ADDRESS)).burn(ajnaRequired);
Copy link
Contributor

Choose a reason for hiding this comment

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

IERC20Token import can be removed

Choose a reason for hiding this comment

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

hey kirill, do we need to concern about non-standard bridged token? or this can be note to deployer to run integration test before deploying it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since AJNA token is already circulating, it should be quite easy to test all success paths on a live L2 deployment with bridged quote, collateral and bwAJNA tokens

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ran on Base yesterday, and Arbitrum, Optimism, and Polygon PoS today. Renamed variables to be less Base-specific, even though token address is hardcoded in the test.

Copy link
Contributor

@prateek105 prateek105 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@MikeHathaway MikeHathaway left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@ith-harvey ith-harvey left a comment

Choose a reason for hiding this comment

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

LGTM

@EdNoepel EdNoepel merged commit d07146c into master Jan 17, 2024
3 checks passed
@EdNoepel EdNoepel deleted the l2-burn branch January 17, 2024 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants