-
Notifications
You must be signed in to change notification settings - Fork 54
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
BAL Hookathon - Swap Bond Hook #89
base: main
Are you sure you want to change the base?
Conversation
Swap hook
Swap hook
Swap hook
refactore code and testcases
@Mubashir-ali-baig is attempting to deploy a commit to the Matt Pereira's projects Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see how much effort went into this! Seems like a useful concept, and a creative use of NFTs. Might be a tad too complex (see comments) :)
* @param _discountCampaignFactory The address of the discount campaign factory. | ||
*/ | ||
constructor( | ||
CampaignDetails memory _campaignDetails, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nit, but _variableName
is for private state variables. When there's a name conflict and we need to alter the name in the signature, we've typically used variableName_
or variableNameParam
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, will be updated in the updated PR
|
||
import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; | ||
|
||
library TransferHelper { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this library needed? Should be able to use @openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol
directly for most of this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right not needed, replaced it with safeERC20
modifier campaignNotExpired(address pool) { | ||
(, , uint256 expirationTime, , , , , ) = IDiscountCampaign(discountCampaigns[pool].campaignAddress) | ||
.campaignDetails(); | ||
if (expirationTime > block.timestamp) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (expirationTime > block.timestamp) { | |
if (block.timestamp <= expirationTime) { |
This is clearer - it hasn't expired if the current time is <= expirationTime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
* @notice Modifier to ensure that the campaign for a given pool has not expired. | ||
* @param pool The address of the liquidity pool. | ||
*/ | ||
modifier campaignNotExpired(address pool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reverts if it hasn't expired, so this name is kind of backwards. You're using it to "restart" a campaign after the current campaign has expired. Consider something like modifier campaignExpired
.
I guess this is the only way to do it, since the hook is immutable once deployed. It's good that you can't change it midstream; that would potentially rug the users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't understand, that this check reverts the update transaction if the owner tries to update the campaign during an ongoing campaign.
* @param tokenAddress Address of the ERC20 token to recover. | ||
* @param tokenAmount Amount of tokens to recover. | ||
*/ | ||
function recoverERC20(address tokenAddress, uint256 tokenAmount) external onlyOwner { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would let the owner remove all the reward tokens, and partial removal would break the discount logic, since it's outside the balance accounting. I don't think anyone would use this with this function here; people just need to not send tokens where they shouldn't. If you just send tokens to the Vault, they're lost (v2 or v3). Same with the factory. Not an obvious security hole there, as the factory shouldn't get any tokens.
You can block people sending ETH if you want (as we do in the v3 Vault). I suppose you could keep track of all the reward tokens, and allow withdrawing tokens that were not rewards, but even that would be hard to do safely (things could be done out of order), and it's just not necessary.
And it would only be correct even then if the owner sent the tokens mistakenly. If somebody else did, you'd have to research and manually return them, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can also allow the owner to remove access tokens apart from the current ongoing reward, but we can remove this function from the campaign contract for the community's trust.
uint256 private _maxBuy; | ||
|
||
/// @notice Maximum discount rate available in the campaign. | ||
uint256 private _maxDiscountRate; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Document the units here. If this is a percentage, we'd typically say _maxDiscountPercentage
, and define it as an 18-decimal FixedPoint value from 0-1. (Here it seems to be used assuming 0-100.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the document and variable name
ISwapDiscountHook private _swapHook; | ||
|
||
/// @notice Maximum buyable reward amount during the campaign. | ||
uint256 private _maxBuy; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a cap on the trade amount; i.e., rewards are proportional to the swap size, but only up to a point, after which it's flat. Maybe swapAmountCap
or something similar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes it is swapAmountCap which is related to the rewardAmount so if the swapAmount exceeds the cap then we'll calculate the reward based on the maxCap.
uint256 newTokenId = _shareTokenId++; | ||
address user = IRouterCommon(params.router).getSender(); | ||
_campaign.updateUserDiscountMapping(_campaignID, newTokenId, user, params.amountCalculatedRaw, block.timestamp); | ||
_mint(user, newTokenId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting to make the swap hook itself an NFT! Minting a new NFT on every swap seems extravagant, though. It would add a fair bit of gas to the critical path, and would get unwieldy for users doing frequent swaps. You could end up with 32 NFTs, and then have to spend the gas to claim them all individually. Probably untenable on mainnet.
The way this is designed, you are rewarding people based on the order of claiming - not the order of swapping - though you do have a cooldown period, so early swappers can claim first. Based on the documentation, this is intended.
But if people are incentivized to claim as soon as possible after swapping, consider simply sending the token "rebate" immediately, vs. going through all this NFT minting and claiming process. That would reward people for swapping early vs. "claiming" early (by essentially doing both simultaneously), but that seems like a valid approach as well. Then we wouldn't need a cooldown period, and the overhead would be just a bit of accounting logic plus a token transfer, and much less onerous for the user.
As it is, an early swapper would have to keep track of the cooldown period, and any who forgot or were busy/distracted could lose the benefit of swapping early if a later swapper claimed first.
You could even enable the swap delta, and return extra tokens right in the swap, though that would prevent this being used on pools that support unbalanced liquidity.
If you want NFTs involved, maybe the Campaign could be an NFT, and you could mint one per user that says they participated in the campaign.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (campaignAddress == address(0)) revert PoolCampaignDoesnotExist(); | ||
(, , , , , , address poolAddress, ) = IDiscountCampaign(campaignAddress).campaignDetails(); | ||
if (pool != poolAddress) { | ||
revert PoolAddressCannotBeChanged(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How could this happen? Should be designed to be impossible, so that you don't need to check for it. (Probably already is; I don't see any way to change the pool address. All you can do is "replace"/"refresh" a campaign with new details, which changes everything at once.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right the second check shouldn't be possible.
Fixed it
rewardAmount: params.rewardAmount, | ||
expirationTime: block.timestamp + params.expirationTime, | ||
coolDownPeriod: params.coolDownPeriod, | ||
discountRate: params.discountAmount, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The amount is the rate? Would make the naming consistent (and a percentage, if that's what it is). For instance, rebatePercentage
(as it's really a rebate, not a discount).
Generally in this system:
A "percentage" is explicitly named as such, as is represented as an 18-decimal FixedPoint value, using the corresponding library (1 = 1e18).
A "rate" is also represented as an FP value, but represents a ratio or something like that - not a "percentage" like a swap fee.
An "amount" (either raw = native token decimals, or scaled18 = 18 decimal FP) is a quantity of something, usually tokens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, all these things would need validation in production. For instance, you'd want to make sure the coolDownPeriod isn't 100 years, percentage values are properly bounded (e.g., <= 100%, or some lower maximum value). Maybe the reward amount is bounded by some minimum/maximum that makes sense, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apart from the name from discount to rebate you are suggesting changing the parameter from discountAmount to discountRate right? Since it's percentage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's a percentage, say "percentage" explicitly (e.g., discountPercentage
or rebatePercentage
). Rates are usually just numbers (e.g., 1.2, 1.05, 0.88) that might represent some kind of ratio or exchange rate, but isn't a percentage. If you mean a literal 0-100% percentage, as you seem to here, it's best to use "percentage" for clarity.
Swap Bond Hook
Swap bond hook is a swap discount utility based hook that equips the pools on balancer v3 to create native incentive campaigns on after swap callback. Projects launching their pools can provide incentives to people buying their token through an incentive campaign in which the token buyers will get a bond nft that represents their buy value. The buyers can redeem reward tokens from the discount campaign based on the value of the token bought.
This contraption will help project attract volume without relying on the 3rd party incentive products and within the safe boundaries of Balancer v3.
Pool Life Cycle Points
This hook primarily uses onRegister and onAfterSwap callback:
Example Use Cases
Suppose Project X is launching XT token on Balancer v3, the project is relatively new and wants to bring volume to the pool. Instead of doing the airdrops or relying on a 3rd party incentive protocol, they integrate the swap bond hook with XT/ETH pool. They create a a campaign of 5000 XT tokens as a reward on 50% initial dynamically decreasing discount on the max trade of 200 XT tokens with a cooldown period of 3 days on reward claim.
This way, the project can essentially create an incentive opportunity for their users while also bringing volume to the project which can kickstart the tokenomics if done right.
This hook is not only for the new pools but the projects can utilise this layer at any stage of their project to invite the influx of token holders.
Code And Architecture Review
Local Test Demo
Discount Formula
For the campaign discount we have opt for a dynamically decreasing discount formula. The projects set an initial discount rate with a deterministic quantity of rewards. The discount rate decreases as the users start redeaming the rewards. Whoever avails the discount first gets a better % as compared to the next user. The update formula demostrate it below.
Calculations of user claimable reward
Intended Project Interaction
For Projects
SwapDiscountHook
with the poolDiscountCampaignFactory
contract and transfer the reward tokens to the campaign.For Users
Challenges
Since Hook based architecture among the Dexes is a fairly new and untested idea as there is no practical example on mainnet. It's uncertain to know from the user experience perspective if it will be adopted widely and how easy it will be for the people to integrate the applications or hooks built on top of the dexes in their daily crypto usage.
While building the swap bond hook, we also had some questions related to how much control should a a project must have on the emission of rewards while making a campaign. It both depends on the tokenomics of that individual project and how much control they would like to have on their emissions.
There is ofcourse a room of improvement as the hook goes into the hands of people and projects.
Running Tests
Run the Foundry tests
DevX Feedback
The online hookathon is well organized through Dorahacks, Kudos on that. The only feedback we have for the team is that they need to figure out more ways for attracting the hackers to contribute to the eco-system either through long term incentives or inclusion with the team.
Authors