feat: LiquidityParty updated adapter#1005
Conversation
| .for_each(|component| match components_store.get_last(&component.id) { | ||
| None => { | ||
| substreams::log::info!( | ||
| "killed component not found in store: {}", | ||
| component.id | ||
| ); | ||
| } | ||
| Some(tokens_str) => match decode_addrs(&tokens_str) { | ||
| Err(e) => { | ||
| substreams::log::info!( | ||
| "failed to decode token addresses for killed component {}: {}", | ||
| component.id, | ||
| e | ||
| ); | ||
| } | ||
| Ok(token_addrs) => { | ||
| for token in token_addrs { | ||
| builder.add_balance_change(&BalanceChange { | ||
| token, | ||
| balance: vec![0], | ||
| component_id: component.id.as_bytes().to_vec(), | ||
| }); | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
Please remove the substreams::log.
I don’t think setting the TVL to zero is a good way to make this pool untradable in Tycho. If a pool is killed, you can update a mutable entity associated with it and filter it out in tycho-simulation.
You can use the Curve filter as a reference. FYI, static attributes are immutable, so they’re not suitable for this kind of kill case.
There was a problem hiding this comment.
Ok, will do.
Any plans in the future to have a PC delete? Seems like the system was designed with CRUD in mind... I thought I saw a DELETE event somewhere although in the previous review I got the impression it wasn't supported. I'll match whatever Curve does.
There was a problem hiding this comment.
Yes, it was designed around CRUD, and supporting deletion would take quite a bit of work. I wouldn’t expect it anytime soon.
There was a problem hiding this comment.
Iterating over all ERC20 Transfer events would add a lot of indexing latency. Does the Liquidity Party pool emit any Swap / Mint / Burn events we could use instead to make the filtering and balance detection more efficient?
There was a problem hiding this comment.
Yes, all of those. I originally had a full suite of our events and switched it out for Transfers last time... You'll notice that this part of the code was not changed in this update. I'll rewrite this to use our custom events.
There was a problem hiding this comment.
Yes, I didn’t notice that in my last review. Switching to the full suite of your events would be a big performance improvement.
There was a problem hiding this comment.
Yah makes total sense. Done.
# Conflicts: # crates/tycho-execution/contracts/test/assets/calldata.txt
I'm going to test the new balance tracking tomorrow. Our test pool already has an "exercise" series of transactions that invokes one of every operation, so I just need to run Substreams on that sequence and verify the final balances. |
|
I found a few more nits in our DEX I'd like to clean up as long as we're re-deploying. The current Tycho adapter should be good to review, but I will probably make one more update to the singleton contract addresses before we're really ready to merge. But no changes to API's or anything like that, just updated deployment addresses. |
|
I updated the contract addresses and start blocks. There was also an ABI touchup, but it doesn't affect anything in Tycho. That's all the updates I wanted to get in. |
|
Our auditor found an issue and we're going to need to redeploy again. Minor issues, nothing close to value extraction. I'd understand if you want to pause review until the audit is complete; should be next week. But the ABI's are stable for swaps and there have been no issues found with swap. The adapter code should be stable as-is except for future contract addr changes. |
Thanks for the heads up. I haven’t seen any other major issues so far, and I’ll take another look once the audit is done. |
|
Turns out we do need to make some pretty significant changes. It's gonna be a few weeks, and I'll probably write a native simulator instead of the EVM simulator. We're moving the bulk of the LMSR solving off-chain and just using an on-chain validation approach instead. Basically we found regimes where our on-chain math wasn't mathing, and the fix is too much gas. So it will be a swap() ABI change to use an offchain solver + on-chain validator model. Don't expect a proper PR for a while. |
Thanks for the heads up! Sounds like you’re planning a native integration? |
|
Our audit will likely wrap up this week, so I should have an updated adapter ready for your review next week. There will not be any significant changes to the adapter. Should just be contract addresses, start blocks, etc. |
|
Not going to be this week; we have a little more work to do. |
swap()interface has changed, so a new Executor will need to be deployed.Killedevents by setting all pool token balances to zero. If we have to disable a pool, this workaround prevents solvers from attempting a swap that would revert, without us needing to support PC deletion.swapToLimit()is no longer supported and was removed from the SwapAdapter.swapAmounts()moved fromPartyPooltoPartyInfoLiquidityPartyExecutorTestwith the exact amount outs expected.PartyPoolSwapImplis calledPartyPoolExtraImplnow.