-
Notifications
You must be signed in to change notification settings - Fork 52
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 - Prediction Market Hook #108
base: main
Are you sure you want to change the base?
Conversation
@shift0x 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. |
@shift0x this looks pretty cool! Great job with the presentation and the video. We'll take some more time to review this properly 🙏 |
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.
Solid work! The manipulation safeguards show that you really thought this through: and the implementation of the hook itself was very simple (as we hope they should be).
I didn't exhaustively go through scenarios to make sure all the math was right everywhere (some numerical examples / diagrams illustrating the different possible outcomes would help here).
using PositionStorage for Position; | ||
|
||
/// @notice fee charged by hook to prediction market participants | ||
uint256 public constant FEE = 10000; // 1% |
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.
Could use an explanation of how 10000 = 1%. Presumably you're doing something like we do with the FEE_SCALING_FACTOR (as we store them in 24 bits). Since there shouldn't be a storage issue here (we had to pack multiple fees into a single word), could you just use 64 (or 256) bits, and store FixedPoint values?
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.
True, I should have a comment here. I'm using 10^6 decimals as the fee precision so 10,000 works out to 1% when i'm doing the fee math.
Definitely could use packed bytes here too. My thought process for using a constant uint256 here was to reduce gas by storing the fee in byte code at compile time, I'd be saving gas by not using storage.
uint256 public constant FEE = 10000; // 1% | ||
|
||
/// @notice waiting period after a swap occurs in a pool and when a market can be settled | ||
uint256 public constant SETTLEMENT_WAITING_PERIOD = 10; |
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.
Consider putting the unit in the name (SETTLEMENT_WAITING_PERIOD_BLOCKS), so people don't think it's seconds.
|
||
emit PredictionMarketHookRegistered(address(this), pool); | ||
|
||
return poolFactory == factory && IBasePoolFactory(factory).isPoolFromFactory(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.
Does this require a specific pool type? It seems like it wouldn't make sense with a stable pool, so this is probably fine (e.g., in production, you could pass in the WeightedPoolFactory). You could then deploy a different hook contract for each kind of WeightedPool you wanted to support (e.g., also the 80/20 one), or have a set of factories the owner could update.
For a minute I thought this was limited to 2-token pools (in which case you could add a check for tokenConfig.length == 2)... but on closer inspection, it looks like this supports any 2 tokens of an n-token pool, but only combinations, not permutations, since you sort the tokens on-chain so people can't make a market for A,B and also B,A.
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.
Weighted pools certainly make the most sense here, but stables can be supported too. I'd only expect markets on stable pools to be useful in the cases where there is a depeg or if traders want to speculate on balance changes between stables. For example USDC <> USDT range in a very tight band around 1.
* @param closedAtTimestamp Timestamp for when the market closes | ||
* @return marketId hashed market id | ||
*/ | ||
function getMarketId( |
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.
Consider computeMarketId
- "get" sounds like it might be checking if it exists (e.g., and returning zero if it doesn't or something), where it's really just computing the bytes32 value of what it would be.
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.
good point, computeMarketId is a better name
} | ||
|
||
/** | ||
* @notice Get a market given it's id |
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.
* @notice Get a market given it's id | |
* @notice Get a market given its id |
Pet peeve :)
* @param self The prediction market | ||
*/ | ||
function isInitalized(PredictionMarket memory self) internal pure returns (bool) { | ||
return self.openPrice > 0; |
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'm sure this is true, but consider an explicit initialization flag for extra clarity.
uint256 amount, | ||
uint256 feePercentage | ||
) private pure returns (uint256 fee) { | ||
return Math.mulDiv(amount, feePercentage, 1e6); |
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.
FixedPoint has a mulDiv too; this is doing some fee scaling factor magic that could stand to be documented.
} else { | ||
(uint256 quoteBull, uint256 quoteBear) = quote(self); | ||
|
||
bullAmount = Math.mulDiv(amount, quoteBull, 1e18); |
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.
Using FixedPoint, this would just be bullAmount = amount.mulDown(quoteBull);
uint256 feeAmount | ||
) internal view returns (uint256 amountOut) { | ||
// revert if the market has 0 liquidity on either side | ||
if(!canSwap(self)){ |
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.
Nit; we test for negatives explicitly for clarity; like canSwap(self) == false
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.
👍 makes sense
constructor( | ||
IVault vault, | ||
address allowedFactory | ||
) VaultGuard(vault) Ownable(msg.sender) { |
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.
It's Ownable, but there aren't any permissioned functions. I can think of some: maintaining a set of allowed factories, in case new WeightedPools or other types are developed that would work here.
Also, what if there's an issue with the pool, and it's paused or put into recovery mode? Perhaps you could have some kind of emergency stop for a market, that would halt a market at current prices and make everything claimable so that people could withdraw. To prevent manipulation by the owner, I imagine this would be permissionless, but would only work if the underlying pool were paused or in recovery.
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.
👍 on what can be Ownable
Good point, I didn't consider that scenario (paused pools / recovery mode) during development. The ability to pause markets in those cases should absolutely be in there.
Asset price prediction markets (binary options in tradfi) are a popular way for market participants to get leverage, hedge or speculate on short term price movements. In their current form, they exist on-chain as services with either partially or fully centralized components.
This hook allows any registered pool to permissionlessly host prediction markets. Participants speculate on whether the price of a pair at some point in the future will be above or below the current price. Each side has a corresponding floating market price governed by UniswapV2 math. At expiration the winners split the balance of the deposited liquidity minus fees.
Hooks are critical to the proper function of the market as it uses incentives to maintain the integrity of the market by giving 0% swap fees to prediction market participants to incentivize arbitrage.
Liquidity providers are compensated by receiving 100% of the swap fees generated from prediction markets. As such, this hook introduces an additional revenue source for liquidity provides that does not depend on price movement. One can think of this style of hook as offering additional value to the ecosystem that participants are willing to compensate LP's for.
How it Brings Value
Example Use Case
A short term trader thinks the market will move higher over the next 25 minutes in reaction to economic news and they want leveraged upside exposure. The trader opens a market and take a bull position paying .5 for each unit. Over time other traders enter the market and prices move. After 25 minutes, the markets close and they end up being correct and now the units are worth 1 -- they made a 2x on their bet.
Feedback about DevX
I had a good experience building this hook. The various examples hooks provided covered how to interact with the contracts and tests so it turned out to be easy to follow. Only downside was needing to use a cloud based machine to work on this since my local macOS is unsupported by foundry