BAL Hookathon - OrderHook #96
Conversation
|
@ExtraCaterpiller 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. |
|
This seems like a really interesting application of hooks and some robust work. It's lacking tests and some way to deploy/interface with it. The https://github.com/balancer/scaffold-balancer-v3 has a lot in place to make this easy. Could you perhaps move this code to a fork of the scaffold-balancer repo and at least add some tests? |
|
I have added some tests and scripts to the PR. Can you please check and verify it now? |
There was a problem hiding this comment.
Thanks for submitting this @ExtraCaterpiller !
I like the idea overall; this would be a very useful feature for pools. And thanks for the readme and the video.
I performed an initial review and I have a few questions. No need to address the comments, but if you could please clarify why you're using token balances as prices I'd appreciate it.
We're evaluating all the submissions at the moment; it'll take a little more time. Thanks again for your participation!
| address tokenIn, | ||
| address tokenOut, | ||
| uint8 slippageTolerance | ||
| ) external returns (bytes32 orderId) { |
There was a problem hiding this comment.
I'd make this one nonReentrant just in case, as there's one external call right in the middle of the function.
There was a problem hiding this comment.
Thank you for the suggestion. Yes, a nonReentrant will make it more secure
| } | ||
|
|
||
| orders[orderId].orderStatus = OrderStatus.CANCELLED; | ||
| IERC20(orders[orderId].tokenIn).transfer( |
There was a problem hiding this comment.
Thanks, I also think so
| prevPrices[params.pool][params.tokenIn] = params.tokenInBalanceScaled18; | ||
| prevPrices[params.pool][params.tokenOut] = params | ||
| .tokenOutBalanceScaled18; |
There was a problem hiding this comment.
Why are token balances stored as "prices"?
Prices depend on the token balances, yes, but also on the pool invariant. Not sure whether I follow the logic here.
There was a problem hiding this comment.
Also, this is not used anywhere else. Is this supposed to be used offchain?
There was a problem hiding this comment.
Thank you for catching that. It seems I initially intended to use these stored balances for managing and executing orders but later changed the architecture, so this code is now redundant. I’ll remove it to streamline the contract and avoid any confusion about its purpose.
| function getPoolPrice( | ||
| address pool | ||
| ) public view returns (uint256[] memory balancesLiveScaled18) { | ||
| balancesLiveScaled18 = i_vault.getCurrentLiveBalances(pool); |
There was a problem hiding this comment.
Here again we're getting the balances and using them as prices directly to check whether we should execute the orders. Could you explain the logic behind it?
There was a problem hiding this comment.
Thank you for the question. My approach here uses token balances as a proxy for prices due to how Balancer pools maintain price ratios relative to token quantities in the pool.
The logic is based on the idea that a change in the relative token balances (especially scaled to 18 decimals for uniformity) reflects price shifts, which can act as a trigger for order execution offchain.
Like after a swap balances change. So, we use the ratio of new balances as price to evaluate existing orders and notify users offchain if their orders are executable.
I think the problem is in the executeOrder() function, where we use the balance of tokenIn in _shouldExecuteOrder and compare it with triggerPrice. We need to send the ratio of tokenIn to tokenOut and then compare with triggerPrice according to current implementation.
However, this approach doesn’t account for the pool’s invariant directly, which could impact accuracy.
There was a problem hiding this comment.
We can use an oracle instead if that works out best for the balancer protocol
|
|
||
| orders[orderId] = Order({ | ||
| orderId: orderId, | ||
| trader: msg.sender, |
There was a problem hiding this comment.
Alright, so only the account who places the order can cancel / execute it; is that correct?
There was a problem hiding this comment.
Yes, that’s correct! Only the account that placed the order has the authority to cancel or execute it.
| // only calls from a trusted routers are allowed to call this hook, because the hook relies on the getSender | ||
| // implementation to work properly | ||
| address private immutable i_trustedRouter; |
There was a problem hiding this comment.
Does it? I don't see any calls to i_trustedRouter.getSender().
There was a problem hiding this comment.
I think it's fine to store a reference if you need it to swap tokens, but I don't see it using the getSender functionality.
There was a problem hiding this comment.
You're correct. In the current implementation, there isn't any use of i_trustedRouter.getSender(). The purpose of i_trustedRouter is to verify that the user initiated the swap through the router contract. The documentation was misleading in this regard. It should be updated as follows:
- // only calls from a trusted routers are allowed to call this hook, because the hook relies on the getSender
- // implementation to work properly
+ // only calls from a trusted routers are allowed to call this hook
address private immutable i_trustedRouter;
OrderHook is a custom Balancer V3 hook designed to automate and enhance the management of token swaps in decentralized exchanges. This contract enables users to define conditional orders, such as stop-loss, take-profit, buy-stop and stop-limit orders, directly within a Balancer pool.
The hook is triggered after a swap occurs in the Balancer pool, capturing key data about the transaction (such as token addresses and their current prices). It emits this data as events, which can be processed by off-chain systems or other smart contracts to implement advanced order execution strategies or to notify users about the status of their orders.