feat: auction noble token #1
Conversation
📝 WalkthroughWalkthroughAdds new AuctionNoble and NobleBurner contracts with interfaces, a Foundry deployment script and Just targets, submodule and remapping updates, optimizer enabled, an updated Changes
Sequence Diagram(s)sequenceDiagram
participant Owner as Owner
participant User as User
participant AuctionNoble as AuctionNoble
participant NobleBurner as NobleBurner
participant NOBLE as NOBLE
Owner->>AuctionNoble: mintToAuction(auction)
Note over AuctionNoble: mintedToAuction = true
User->>AuctionNoble: transfer(to, amount)
alt mint/burn/to-contract
Note over AuctionNoble: normal ERC20 flow
else other transfer
AuctionNoble->>NOBLE: transfer equivalent NOBLE to NobleBurner
NobleBurner->>NOBLE: doBurn() (calls burn)
Note over NOBLE: NOBLE.burn() destroys tokens
AuctionNoble-->>User: emits Burned(from, amount) and burns wrapper tokens
end
Owner->>AuctionNoble: recoverUnsold()
AuctionNoble->>NOBLE: transfer remaining NOBLE to Owner (after burning wrapper)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/AuctionNoble.sol`:
- Line 31: ERC20Burnable's burn and burnFrom currently call _burn which triggers
AuctionNoble._update and short-circuits on to == address(0) so the underlying
NOBLE isn't burned; override burn(address) and burnFrom(address,uint256) in
AuctionNoble to perform the wrapper token burn while explicitly calling the path
that burns underlying NOBLE (i.e. invoke the internal flow that calls
underlyingNobleToken.burnFrom or equivalent and then update balances), or
alternatively implement the overrides to call a new internal function (e.g.,
_burnWithUnderlying) that calls _burn (or _update) and also executes the
underlying NOBLE burn, ensuring you reference and use the existing _update and
_burn semantics so the 1:1 invariant is preserved.
🧹 Nitpick comments (4)
.env.example (1)
1-2: Add documentation comments and consider additional deployment variables.The environment variables lack inline comments explaining their purpose and expected format. Consider:
- Adding comments to describe what each variable is for (e.g.,
# Etherscan API key for contract verification)- Providing example placeholder values (e.g.,
OWNER=0x1234...)- Including deployment-related variables that Foundry scripts typically require, such as
RPC_URLfor the target network and potentiallyPRIVATE_KEYorMNEMONICfor transaction signing📝 Suggested improvements
+# Etherscan API key for contract verification ETHERSCAN_API_KEY= + +# Owner address for the AuctionNoble contract OWNER= + +# RPC URL for deployment (e.g., https://eth-mainnet.g.alchemy.com/v2/YOUR-API-KEY) +RPC_URL= + +# Private key for deployment transactions (keep secure, never commit) +PRIVATE_KEY=.gitignore (1)
4-7: Consider removing the empty line for consistent formatting.The empty line at line 5 creates inconsistent spacing compared to the rest of the file. Standard
.gitignoreformatting typically groups related entries without extra blank lines, or uses blank lines to separate logical sections consistently.♻️ Suggested formatting
broadcast - .vscode .envOr, if you prefer grouping by category, consider:
+# Build and dependency artifacts cache node_modules out + +# Deployment artifacts broadcast +# IDE and environment .vscode .envscript/DeployAuctionNoble.s.sol (1)
12-21: Add a zero-address guard for OWNER to prevent accidental mis-deploys.
A simple check makes misconfiguration safer during deployment.♻️ Suggested guard
- address owner = vm.envAddress("OWNER"); + address owner = vm.envAddress("OWNER"); + require(owner != address(0), "OWNER must not be zero");src/AuctionNoble.sol (1)
46-48: Validate burner address on construction.A zero/EOA burner would silently accept underlying transfers without burning. Consider a guard to prevent misconfiguration.
♻️ Suggested guard
constructor(address _owner, address _burner) ERC20("AuctionNoble", "NOBLE") Ownable(_owner) { + require(_burner != address(0), "Burner is zero"); + require(_burner.code.length > 0, "Burner not a contract"); BURNER = INobleBurner(_burner); }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/AuctionNoble.sol`:
- Around line 43-48: Add validation in the AuctionNoble constructor to prevent a
bad burner address: require that _burner is not the zero address and not the
token contract itself, and verify it's a contract (use OpenZeppelin's
Address.isContract or inline extcodesize) before assigning BURNER =
INobleBurner(_burner); e.g., in the constructor check (_burner != address(0) &&
_burner != address(this) && Address.isContract(_burner)) and revert with a clear
message if the check fails so doBurn() cannot be pointed at an EOA/zero/self
address.
|
It was not clear to me directly by reading the code, but how are we keeping track of which user participated to the auction and how much they bought? Are we using an offchain indexer? |
Yes, the idea is that we index the |
MalteHerrmann
left a comment
MalteHerrmann
left a comment
There was a problem hiding this comment.
looks good, will run some tests as well
src/AuctionNoble.sol
Outdated
|
|
||
| /// @notice Initializes the AuctionNoble wrapper token. | ||
| /// @param _owner The address that will own this contract and can call admin functions. | ||
| constructor(address _owner) ERC20("AuctionNoble", "NOBLE") Ownable(_owner) { |
There was a problem hiding this comment.
is there any potential confusion from the fact that we're deploying this with the same symbol as the original NOBLE token? why not use NOBLE.a or ccaNOBLE or smth instead?
There was a problem hiding this comment.
is that a requirement for the auction so that users will see the correct NOBLE symbol when interacting with the auction contracts?
There was a problem hiding this comment.
good quesiton, actually going to forward this to Product and see what they want for this wrapper token
There was a problem hiding this comment.
after bringing up with product we are using NOBLEV for symbol and Noble Voucher for the name
AuctionNoble token that will be used in the Uniswap CCA auction.
It's basically a wrapper around Noble to allow burning on transfers when users claim from the auction, how it works:
mintToAuctionfunction that checks the entire contract's Noble balance and mints the equivalent wrapper tokens into the auction address provided (can only be called once!).recoverUnsoldwhich will burn the wrapper and return the Noble to the OWNER address.The only requirement to make it work with the Uniswap CCA contract is that we create the auction with this field
set as the wrapper contract, so that they are not burned during the unsold tokens recovery.
Tested here
https://github.com/noble-assets/continuous-clearing-auction/blob/noble-auction/script/test/AuctionNoble.s.sol
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.