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

feat: set burn amounts for upgrading flow #8

Merged
merged 2 commits into from
May 8, 2024
Merged

Conversation

excaliborr
Copy link
Contributor

@excaliborr excaliborr commented May 8, 2024

🤖 Linear

Closes OPT-32

Copy link

linear bot commented May 8, 2024

OPT-32 add `burnLockedUSDC()` to the BaseOpUSDCBridgeAdapter

  • Add a setter function called setBurnAmount()
  • burnLockedUSDC should use a storage variable
  • only callable by the owner
  • 100% Unit test coverage

@excaliborr excaliborr marked this pull request as ready for review May 8, 2024 13:58
Copy link
Member

@hexshire hexshire left a comment

Choose a reason for hiding this comment

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

Small comment.

* @dev Only callable by the owner
*/
function setBurnAmount(uint256 _amount) external onlyOwner {
burnAmount = _amount;
Copy link
Member

Choose a reason for hiding this comment

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

Not necessary but we could add a check to avoid setting the current amount again if(burnAmount == _amount) revert()

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 a better approach would be without the revert

if (burnAmount != _amount) {
...
}

But agree that's not very necessary.

* @notice Burns the USDC tokens locked in the contract
* @dev The amount is determined by the burnAmount variable, which is set in the setBurnAmount function
*/
function burnLockedUSDC() external;
Copy link
Contributor

Choose a reason for hiding this comment

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

Linter warning here. However, I can push a PR later fixing all the warnings

Copy link
Contributor Author

@excaliborr excaliborr May 8, 2024

Choose a reason for hiding this comment

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

ye we have a bunch cause of mixed case, I will merge this and we can adjust the linter in a future PR, the styling and order was copied from the boilerplate its because in contracts we make view be after logic but variables are technically view functions in interfaces


import {IERC20} from '@openzeppelin/contracts/token/ERC20/IERC20.sol';

interface IUSDC is IERC20 {
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this 👍

Copy link
Contributor

@0xDiscotech 0xDiscotech left a comment

Choose a reason for hiding this comment

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

🔥

@excaliborr excaliborr merged commit e39a698 into dev May 8, 2024
4 checks passed
@excaliborr excaliborr deleted the feat/set-burn-amounts branch May 8, 2024 18:28
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.

3 participants