Repository navigation
Conversation
GitHub Actions
Failure
GitHub Actions
Failure
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Failure
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Failure
GitHub Actions
Error
GitHub Actions
Failure
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Failure
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
|
@zach030 rebased onto current main and resolved the conflicts. Updated the generated BaiBai test calldata for the changed upstream executor setup; all six fork tests, native/encoder/Substreams tests and scoped Clippy pass. Could you take another look? |
zach030
left a comment
zach030
left a comment
There was a problem hiding this comment.
Thanks! Looks good! Just left a few comments.
|
Heads-up: the BaiBai CurveBook was upgraded on Base at block 51696183 (tx 0xc9d0e733da27ab34e42e499e704b881b39fda83a4cbe1e4545d2bb41e666ef5b). The new implementation, 0xAd6beCedB2c4b898CA9d4dE248Dc7207fe4d1C80, is source-verified. What changed: the book now emits an explicit pair-creation event: event PairListed(address indexed base, address indexed quote);A base's first successful SHAPE emits it. The upgrade itself backfilled it once for WETH/USDC through a one-shot Effect on this PR: 51696183 is past
|
zach030
left a comment
zach030
left a comment
There was a problem hiding this comment.
Thanks for the update! This round, I mainly focused on performance and extensibility, so I left a few suggestions for improvement.
| if let Some(base) = delta.key.strip_prefix("pair:") { | ||
| store.append(delta.ordinal, "pairs", base.to_string()); | ||
| } else if let Some(fee) = delta.key.strip_prefix("fee:") { | ||
| let (base, taker) = fee.split_once(':').unwrap(); | ||
| store.append(delta.ordinal, format!("fees:{base}"), taker.to_string()); | ||
| } |
There was a problem hiding this comment.
How many pairs and fee entries do you expect on Base? Since Substreams limits store values to 8 KB, we can’t append to the store indefinitely.
I think fees:{base} is the main concern, since you may need to whitelist a large number of taker addresses.
There was a problem hiding this comment.
We expect very few: one pair (WETH-USDC) today and single digits later, with fee overrides only for a handful of routers. To remove the ceiling anyway, taker lists are now sharded by the taker's first byte (fees:{base}:{xx}), which gives roughly 50k takers per base before any shard nears 8 KB. pairs stays a single list (~190 bases), which is well above what we plan to list against one quote.
| if log.address == config.custodian.as_slice() && | ||
| (topic == settled.as_slice() || topic == executed.as_slice()) | ||
| { | ||
| let token = U256::from_be_slice(&log.topics[2]); | ||
| claim_slots.insert(B256::from(mapping(token, claims))); | ||
| } |
There was a problem hiding this comment.
Are Settled and Executed the only events that update claims[token]? Is there any other way it could change that we haven’t covered?
There was a problem hiding this comment.
Yes. That mapping (totalClaimable[token]) is written only in settleWithdraw (including the zero-amount cancel) and executeWithdraw. They emit WithdrawSettled and WithdrawExecuted respectively, both with token as topic 2. Nothing else writes it, and an implementation change would emit Upgraded, which pauses the component.
| const TAKER_FEE_SET: B256 = | ||
| b256!("1f50e1aaaff835659bf08a8d3473edefb7f61e28a27572c62fa8daa40da9e268"); | ||
| const TAKER_FEE_CLEARED: B256 = | ||
| b256!("4e36b92da5e2a98be73ea9f9bd228101bb4b4e83629908148cef524aaad69b94"); | ||
| const UPGRADED: B256 = b256!("bc7cd75a20ee27fd9adebab32041f755214dbc6bffa90cc0225b39da2e5c2d3b"); |
There was a problem hiding this comment.
Can we define all event signatures as constants here? There are other topics in state_changes that are also generated with keccak256.
There was a problem hiding this comment.
Done. All event topics are now b256! constants.
| let bases = addresses(keys, u64::MAX, "pairs"); | ||
| let previous: BTreeSet<_> = addresses(keys, 0, "pairs") | ||
| .into_iter() | ||
| .collect(); |
There was a problem hiding this comment.
We can use the store_state deltas to get the updated tokens, so we don’t need to read it twice and filter them here.
There was a problem hiding this comment.
Done. map_balances now takes store_state deltas and bootstraps the bases whose pair: key was created in this block, so the get_first/get_last double read is gone.
| fn addresses(read: ReadStore<'_>, ordinal: u64, key: &str) -> Vec<Address> { | ||
| read(ordinal, key).map_or_else(Vec::new, |value| { | ||
| // Written only by store_keys; append's wire encoding is semicolon-delimited. | ||
| String::from_utf8(value) | ||
| .unwrap() | ||
| .split(';') | ||
| .filter(|v| !v.is_empty()) | ||
| .map(|v| v.parse().unwrap()) | ||
| .collect() | ||
| }) | ||
| } |
There was a problem hiding this comment.
The addresses function is used in several places and behaves differently for different ordinals. Can we simplify this, or would it make sense to wrap it in a helper function?
There was a problem hiding this comment.
Simplified. addresses is now a plain parser for a store_keys value, and callers read the store themselves (get_last("pairs") in the handlers, get_at for fee shards), so there's no ordinal-dependent behaviour left.
| for &base in &bases { | ||
| if state(tx.end_ordinal, &format!("pair:{base:x}")).is_none() { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The pair:{base:x} store only changes once when the pair is created, right? IMO, we can add an early filter so we don’t have to scan every base.
There was a problem hiding this comment.
Done. listings() collects the pair: creation ordinals from the deltas once per block. A base is skipped until its listing ordinal and treated as created within that tx, so the per-base, per-tx store read is gone.
| attrs | ||
| } | ||
|
|
||
| fn protocol_changes( |
There was a problem hiding this comment.
Can you try simplifying this function? It has high cyclomatic complexity and some redundant reads and loops, so I think there’s room to simplify it.
There was a problem hiding this comment.
Simplified. Each tx now takes its prefix of the (ordinal-ordered) deltas instead of filtering the full lists per tx, pause/creation come straight from the deltas, and balance changes use the delta's new_value. The balance store is only read on creation.
zach030
left a comment
zach030
left a comment
There was a problem hiding this comment.
Ran the range test locally end to end, all tests work fine! Left a few suggestions.
| pub fn sync_custody(&mut self, from: &Self) { | ||
| for (i, token) in self.tokens.iter().enumerate() { | ||
| if let Some(j) = from | ||
| .tokens | ||
| .iter() | ||
| .position(|other| other == token) | ||
| { | ||
| self.balances[i] = from.balances[j]; | ||
| self.words[30 + i] = from.words[30 + j]; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Can we remove sync_custody? Tycho doesn't model cross-pool coupling anyway (RFQ makers also share inventory across pairs), and the worst case is a revert since the router enforces minAmountOut. I'd remove it along with the test, and just note the limitation in the README.
There was a problem hiding this comment.
Removed in ec83f4a: sync_custody and its test are gone, along with the static custodian attribute that only existed to group pools for it (balance_owner still identifies the custodian). The README now notes that pairs sharing the custodian are simulated independently, so the worst case is a revert enforced by the router's minAmountOut.
| for fee_base in [base, Address::ZERO] { | ||
| for taker in (0..=u8::MAX).flat_map(|shard| { | ||
| addresses(keys(ordinal, &fees_key(&format!("{fee_base:x}"), &format!("{shard:02x}")))) | ||
| }) { | ||
| let value = state(ordinal, &format!("fee:{fee_base:x}:{taker:x}")).unwrap(); | ||
| attrs.push(attribute( | ||
| &format!("{}_fee_{taker:x}", if fee_base.is_zero() { "taker" } else { "pair" }), | ||
| value, | ||
| true, | ||
| )); | ||
| } | ||
| } |
There was a problem hiding this comment.
Passing the shard as taker only works because fees_key slices &taker[..2], which is a bit confusing. Suggest a fee_shard_key(base, shard: u8) + plain loops. Also, could you add a second taker with a different first byte to the tests to cover cross-shard reads?
for fee_base in [base, Address::ZERO] {
let base_hex = format!("{fee_base:x}");
let kind = if fee_base.is_zero() { "taker" } else { "pair" };
for shard in 0..=u8::MAX {
for taker in addresses(keys(ordinal, &fee_shard_key(&base_hex, shard))) {
let value = state(ordinal, &format!("fee:{base_hex}:{taker:x}"))
.expect("store_keys only lists takers with a fee entry");
attrs.push(attribute(&format!("{kind}_fee_{taker:x}"), value, true));
}
}
}There was a problem hiding this comment.
Done in 983415a: added fee_shard_key(base, shard: u8), used by both store_keys (shard = the taker's first byte) and the snapshot reader, which now uses your plain-loop shape. Stored keys are unchanged. The discovery test adds a second taker (0xa7…) with pair and taker-wide fees in a different shard, and fails if the reader skips that shard.
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the PropellerHeads CLA and I hereby sign the CLA |
|
@zach030 rebased onto current
Your Oct 5 suggestions went in with the previous push ( The fork CI runs are waiting on maintainer approval ( |
Tycho simulates components independently, so sync_custody and the static custodian attribute that grouped pools for it are removed. The README notes that routes through several pairs may overestimate shared custody; the router's minAmountOut bounds the outcome to a revert.
Replace fees_key's taker-prefix slicing with fee_shard_key(base, shard), used by both store_keys and snapshot reads. Stored keys are unchanged. Cover snapshot reads across two shards.
Upstream renamed the contracts' SPDX license; scripts/check-licensing.sh rejects the remaining BUSL-1.1 headers.
|
Rebased again onto |
This branch is waiting to be deployed
2 participants
Add indexing, native exact-input simulation and swap execution for BaiBai CurveBook v3 on Base. The indexer discovers base-token pairs from successful curve publications on the configured deployment, retaining pre-listing fee, storage and custody history. Pair IDs combine the entrypoint and base token; all pairs use the deployment's quote token.
Custody is tracked once per token and propagated to every affected pair, together with shared claim reservations, TTL and taker-wide fees. A new base uses one block-pinned balance bootstrap that accounts for earlier deposits and same-block transfers. Normal updates and quotes require no RPC calls. Each pair is simulated independently, like other venues whose makers share inventory across pairs; the README notes that a route through several BaiBai pairs can overestimate a shared balance, with a revert as the worst case since the router enforces
minAmountOut.balance_owneridentifies the custodian.Fees follow the deployed contract: executing taker's pair override, then taker-wide override, then zero. Explicit zero overrides remain meaningful.
DecoderContext::callersupports custom execution callers and defaults to the Tycho Base router. Integer rounding, gross curve consumption, net custody payouts and normal router slippage protection are preserved.Addressed the remaining review items: removed the redundant
fees_indexedmarker, local simulator pause handling and unused testing dependency. Dependency upgrades after the validated block still emit Tycho's standard pause attribute, including for later-discovered pairs. Generic client removal of paused components is an upstream follow-up; this PR does not duplicate that logic. The separate historical-retention fix also remains upstream-owned and is applied only in the isolated replay checkout.The CurveBook proxy was upgraded at Base block 51,696,183. The new implementation adds a
PairListed(base, quote)event, emitted on a base's first curve publication and once for WETH during the upgrade; immutables (entrypoint, quote token, curve unit), storage layout and curve/fill/quote logic are unchanged. After checking the verified source and deployed bytecode, the validated block is now 51,696,183. Pair discovery still uses the firstCurveUpdated, because WETH's history predatesPairListed; the adapter ignores the new event.Validation:
mainat b5826a2. BaiBai is now listed inRuntimeBytecodeFixtures.sol, pinned at Base block 51,696,183 because the executor constructor readsquoteToken()from the entrypoint, which postdates the shared Base fork block. Upstream's builder reproduces the committedBaibai.runtime.jsonbyte for byte. Upstream's newEncodingContext::originfield is set in the BaiBai encoder tests, and the test executor address was refreshed for the new executor deployment order. Canonical Rust CI unit command: 1,782 passed. Root and Substreams Clippy, formatting, WASM build, 11 native, 8 Substreams and encoder/calldata-generator tests, and all six BaiBai fork tests passed.PairListedlog at 51,696,183, passed: nine runner checks, the full integration harness and six exact simulation/execution matches (three sizes in each direction). An independent database comparison matched all 32 storage words and both custody balances, checked complete fee-event history (no overrides in this range), confirmed the removed marker is absent and found no pause attribute. Nonzero overrides and fee changes are covered by synthetic tests. The replay database dump checksum was verified and disposable services were stopped.The package is still unreleased at 0.1.0. The changed module graph requires indexing from the manifest's initial block. A production executor deployment and indexing registration are still required; no production executor address is assigned here.