feat: Season 1 Airdrop Implementation #149
Conversation
| @@ -18,8 +18,11 @@ import { AA_ClaimingNotExpired, AA_ClaimingExpired, AA_ZeroAddress } from "../er | |||
| * The contract is ownable, where the owner can reclaim any remaining tokens once the airdrop is | |||
| * over and also change the merkle root and its expiration at their discretion. | |||
| */ | |||
| contract ArcadeAirdrop is ArcadeMerkleRewards, Authorizable { | |||
| abstract contract ArcadeAirdropBase is ArcadeMerkleRewards, Authorizable { | |||
There was a problem hiding this comment.
What is the motivation for separating ArcadeAirdropBase and ArcadeMerkleRewards? I'm not sure what other contracts would inherit ArcadeMerkleRewards
| // stake tokens in voting vault for this msg.sender and delegate | ||
| votingVault.airdropReceive(msg.sender, totalGrant, delegate, lock); | ||
|
|
||
| emit ClaimAndStaked(msg.sender, totalGrant, delegate, lock); |
There was a problem hiding this comment.
Should probably just emit Claim here, all claims should emit a unified event or it adds overhead to all tracking/notification hooks (which may or may care if something is staked after or not).
You could emit two events, both Claim and ClaimAndStake, but IMO the latter is not needed because the staking contract will emit its own Deposit event
| // must be before the expiration time | ||
| if (block.timestamp > expiration) revert AA_ClaimingExpired(); | ||
| // validate the withdraw | ||
| _validateWithdraw(totalGrant, merkleProof); |
There was a problem hiding this comment.
Not sure a better name, but I was a bit surprised here that _validateWithdraw was not view, in my mind "validate" in a function name implies behavior that it will check values and error, so the storage update to claim was a bit of a surprise
There was a problem hiding this comment.
Yeah I could pull the state change in claim out to the inherting contract, but was trying to be as least invasive with the changes since I was changing audited code. Same with the ArcadeStakingRewards contract that was how it was done originally and since it was audited was trying to lower my risk by not moving too much around.
But what I am gathering from these comments is I should take it one step further and make it as clean as possible by making validations view and have a combined ArcadeAirdropBase contract. Will make these changes.
|
Summary of changes made after first code review. |
Modify the airdrop inheritance structure to allow for more composable future airdrop scenarios. A new folder named
contracts/token/airdropnow holds the airdrop implementation contracts. Additionally in this folder, there is a ArcadeAirdropBase contract that inherits ArcadeMerkleRewards. The ArcadeAirdropBase contract should be inherited by all airdrop implementation (season) contracts.For the latest airdrop season, season 1, users have two claiming options. Either claim the airdrop directly to their wallets, or claim to the ArcadeSingleSidedStaking vault to accrue points and access voting power.
All scripts and tests have been updated to reflect these changes.