feat: index routed-deposit destinationOwner on Deposit - #7
Conversation
|
Warning Review limit reached
Next review available in: 24 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughRouted depositor ABIs, schema fields, mapping handlers, network addresses, and five subgraph data sources were added. Current and legacy event variants attach destination ownership and sender data to existing ChangesRouted deposit indexing
Sequence Diagram(s)sequenceDiagram
participant RoutedDepositor
participant mappingRoutedDeposit
participant Deposit
RoutedDepositor->>mappingRoutedDeposit: Emit routed deposit event
mappingRoutedDeposit->>mappingRoutedDeposit: Normalize owner and derive deposit id
mappingRoutedDeposit->>Deposit: Update destinationOwner, l1Sender, and isRouted
Deposit-->>mappingRoutedDeposit: Persist entity
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/mappingRoutedDeposit.ts`:
- Around line 30-32: Update leftPadAddressTo32Bytes to use a Bytes-typed 12-byte
padding value before concatenating with address, ensuring the returned
concatenation is accepted as Bytes rather than ByteArray.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 31d5d11f-3076-40a3-8af5-1dcc6b24b0b3
📒 Files selected for processing (5)
abis/RoutedDepositor.jsonnetworks.jsonschema.graphqlsrc/mappingRoutedDeposit.tssubgraph.yaml
| function leftPadAddressTo32Bytes(address: Bytes): Bytes { | ||
| return new Bytes(12).concat(address) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
curl -fsSL \
https://registry.npmjs.org/@graphprotocol/graph-ts/-/graph-ts-0.31.0.tgz |
tar -xz -C "$tmp"
rg -n -A4 -B4 'concat\(other: ByteArray\)' "$tmp/package/common"Repository: threshold-network/tbtc-subgraph
Length of output: 956
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Tracked files matching mappingRoutedDeposit.ts:"
git ls-files | grep -E '(^|/)mappingRoutedDeposit\.ts$' || true
file="$(git ls-files | grep -E '(^|/)mappingRoutedDeposit\.ts$' | head -n 1)"
if [ -n "${file:-}" ]; then
echo
echo "File: $file"
wc -l "$file"
echo
sed -n '1,80p' "$file" | cat -n
fi
echo
echo "Lock/package graph-ts references:"
rg -n '"`@graphprotocol/graph-ts`"|graph-ts' package.json package-lock.json yarn.lock pnpm-lock.yaml 2>/dev/null || trueRepository: threshold-network/tbtc-subgraph
Length of output: 4602
🌐 Web query:
AssemblyScript class inheritance subclass as superclass return type method compatible
💡 Result:
In AssemblyScript, method overriding and return type compatibility follow strict rules influenced by the language's static compilation and the lack of traditional virtual function lookup found in higher-level languages [1]. Regarding the return type of overridden methods: - Historically, using this as a return type in a base class method was problematic because the compiler resolved it as the base class type rather than the inheriting subclass type [2][3]. - Updates have been made to support the polymorphic this type on class methods [3]. If you define a method to return this, the compiler is intended to resolve it to the calling instance type [3]. - If you encounter issues where an overridden method is not correctly returning the subclass type, ensure you are using a recent version of the AssemblyScript compiler, as improvements to this type resolution were implemented to address this specific limitation [2][3]. Important considerations for inheritance in AssemblyScript: - Type Casting: AssemblyScript supports upcasting (subclass to superclass) implicitly or with the as operator [4][5]. For downcasting (superclass to subclass) or casting between siblings, you must use changetype<T>(...), as the compiler does not perform runtime type checks for these casts [5]. - Overriding: AssemblyScript does not implement standard virtual function lookups; therefore, overriding methods must be carefully managed [1]. When a subclass overrides a method, it is best practice to ensure the signature matches precisely. Issues can arise when attempting to override methods with incompatible types or mixing generic and non-generic method signatures [6][7]. - Properties: Note that overriding class properties is not directly supported in the same way as methods; developers often use the constructor to initialize or reassign inherited properties as a workaround [8]. For the most reliable behavior, always explicitly define return types and avoid relying on complex implicit polymorphism, as the language prioritizes clear, static resolution to maintain WebAssembly performance [9][1].
Citations:
- 1: https://wasmbyexample.dev/examples/classes/classes.assemblyscript.en-us.html
- 2: https://stackoverflow.com/questions/79232050/can-i-return-this-from-a-base-class-instance-method-in-assemblyscript
- 3: feat: support 'this' type on methods and properties AssemblyScript/assemblyscript#2906
- 4: https://stackoverflow.com/questions/71301884/assemblyscript-class-inheritance-type-casting
- 5: https://github.com/TheGraphAcademy/Graph-Academy-Hub/blob/main/official-docs/developer/assemblyscript-migration-guide.md
- 6: compiler crashed when generic function override non generic function AssemblyScript/assemblyscript#3013
- 7: AssemblyScript/assemblyscript@2180c8c
- 8: Subclass can not override superclass property AssemblyScript/assemblyscript#1091
- 9: https://www.assemblyscript.org/types.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
VERSION="$(sed -n 's/^ "version": "\(.*\)",/\1/p' package.json | head -n 1)"
VERSION="${VERSION:-19.0}"
if [ -x "$(command -v npm)" ]; then
npm --silent pack "assemblyscript@$VERSION" > "$tmp/npm-pack-name"
tar -xzf "$tmp/$(cat "$tmp/npm-pack-name")" -C "$tmp"
echo "Packed and extracted AssemblyScript@" >&2
else
echo "npm unavailable" >&2
exit 0
fi
cat > "$tmp/concat-as-upcast.as" <<'AS'
class ByteArray {
length: i32
constructor(length: i32) { this.length = length }
// Type-safe concat matching our code is not required for this probe;
// the compiler rejects returning a subclass where a superclass is expected.
concat(_: ByteArray): ByteArray { return changetype<ByteArray>(this) }
}
class Bytes extends ByteArray {
concat(_: ByteArray): Bytes { return changetype<Bytes>(this) }
}
function leftPadAddressTo32Bytes(address: Bytes): Bytes {
return new ByteArray(12).concat(address)
}
export function result(): Bytes {
return leftPadAddressTo32Bytes(changetype<Bytes>(new ByteArray(20)))
}
AS
node "$tmp/assemblyscript-$VERSION/bin/asc.js" "$tmp/concat-as-upcast.as" --exportRuntime --textFile /dev/null \
2>&1 | sed -n '1,80p'
echo
echo "If the return type matches via subclass-to-superclass upcast, removing this block would be appropriate:"
python3 - <<'PY'
from pathlib import Path
p = next(Path('/tmp/asm-as-upcast').glob('package/*'), None)
print('assemblyscript package location:', p)
print('assemblyscript.js exists:', (p / 'assemblyscript.js').exists())
PYRepository: assemblyscript/assemblyscript
Length of output: 388
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Dependencies:"
cat package.json | python3 - <<'PY'
import json, sys
data=json.load(sys.stdin)
for sect in ['dependencies','devDependencies','packageManager']:
if sect in data:
print(f'[{sect}]')
for k,v in data[sect].items():
if 'assemblyscript' in k.lower() or 'graph-ts' in k.lower():
print(f'{k}: {v}')
PY
echo
echo "Search for assemblyscript/graph-ts config/tooling:"
git ls-files | rg -i 'assemblyscript|graph-ts|tsconfig|yarn|package-lock|pnpm' || true
echo
echo "Inspect relevant tsconfig files:"
for f in $(git ls-files | rg -i 'tsconfig.*\.json$'); do
echo "--- $f"
sed -n '1,160p' "$f" | cat -n
doneRepository: threshold-network/tbtc-subgraph
Length of output: 904
🌐 Web query:
AssemblyScript return subclass where base class expected type error
💡 Result:
In AssemblyScript, encountering a type error when returning a subclass where a base class is expected—or when expecting a subclass type from a method declared to return a base class—often stems from differences between AssemblyScript's strict type system and TypeScript's more flexible, often bivariant, type system [1][2]. If you are experiencing issues where a method returning this or a subclass is not recognized as the subclass type, here are the primary factors and workarounds: 1. Type Inference Limitations: AssemblyScript requires explicit type annotations because its type inference is more limited than TypeScript's [2]. The compiler does not always automatically narrow return types to a subclass, even if the runtime object is an instance of that subclass [1]. 2. Method Return Types: If a base class method is declared to return the base type (e.g., : Base), the compiler treats the return value as the base type [1]. Even if you return this (which can be typed as this in some contexts), usage of subsequent subclass-specific properties may fail because the compiler's static analysis only sees the base class definition [1]. 3. Explicit Casting: Unlike TypeScript, which allows for implicit subtyping in many scenarios, AssemblyScript often requires explicit casting to treat a base class instance as a subclass [3]. Use the as operator for downcasting: const mySubclass = myBaseInstance as Subclass; [3]. 4. Alternative Pattern: To avoid type errors, ensure your methods are explicitly typed to return the specific subclass, or use generics if you are writing reusable base class logic that needs to return the derived type [4]. AssemblyScript also has known limitations regarding inheritance, such as inability to easily override class properties [5] and specific challenges with how this is resolved in static contexts [6]. If you believe the compiler is failing to recognize a valid subclass return in a way that should be supported, verify if your code triggers one of the documented compiler restrictions or report it as an issue if it diverges from standard behavior [1].
Citations:
- 1: https://stackoverflow.com/questions/79232050/can-i-return-this-from-a-base-class-instance-method-in-assemblyscript
- 2: https://www.assemblyscript.org/types.html
- 3: https://stackoverflow.com/questions/71301884/assemblyscript-class-inheritance-type-casting
- 4: https://stackoverflow.com/questions/58573975/can-typescript-return-the-subclass-if-a-function-has-the-superclass-as-its-retur
- 5: Subclass can not override superclass property AssemblyScript/assemblyscript#1091
- 6: Incompatible with TS case with static fields combine with inheritance AssemblyScript/assemblyscript#1364
🌐 Web query:
"@graphprotocol/graph-ts" "ByteArray.concat" "0.31.0" "Bytes"
💡 Result:
In the @graphprotocol/graph-ts library, including version 0.31.0, the ByteArray.concat method is an available API used to combine byte arrays [1][2][3]. The ByteArray.concat(other: ByteArray): ByteArray method returns a new ByteArray consisting of the original ByteArray followed directly by the other ByteArray provided as an argument [1][2][3]. Additionally, the library supports the following related concatenation operations: - concatI32(other: i32): ByteArray: Returns a new ByteArray consisting of the original ByteArray followed by the byte representation of the provided i32 integer [1][2][3]. - b.concat(other: Bytes): Bytes: For Bytes objects, this returns a new Bytes object consisting of the original Bytes followed by the other Bytes [1][3]. These functions are part of the AssemblyScript API for writing subgraph mappings [2][4]. The library's development has been unified within the graph-tooling monorepo [4].
Citations:
- 1: https://github.com/graphprotocol/docs/blob/main/website/src/pages/en/subgraphs/developing/creating/graph-ts/api.mdx
- 2: https://thegraph.com/docs/en/subgraphs/developing/creating/graph-ts/api/
- 3: https://thegraph.academy/docs-sub/official-docs/developer/assemblyscript-api/
- 4: https://github.com/graphprotocol/graph-ts
🌐 Web query:
AssemblyScript "ByteArray" "Bytes" "@graphprotocol/graph-ts"
💡 Result:
In the @graphprotocol/graph-ts library used for writing subgraph mappings, both ByteArray and Bytes are classes used to handle byte data, but they serve slightly different semantic purposes [1][2]. ByteArray The ByteArray class is designed to represent a generic array of bytes (u8) [1][3]. It provides utility methods for common operations such as conversion to/from hexadecimal strings, base58 encoding, and conversion to numeric types (u32, i32) [1][2]. Bytes The Bytes class is a subclass of ByteArray and is specifically used to represent arbitrary-length arrays of bytes, such as Ethereum types like bytes and bytes32 [1][4]. Because it extends ByteArray, it inherits all of its functionality [1][4]. In practice, Bytes is the standard type used for handling Ethereum-related byte values, such as contract addresses and event data [1][5]. Key Technical Points Relationship: Since Bytes extends ByteArray (and both extend the standard AssemblyScript Uint8Array), they share a common base [1][4]. Casting: Because they share a superclass (Uint8Array) but are distinct classes, direct casting using the 'as' keyword is often not safe [6][7]. The library provides the changetype function for safe conversion between these types, as well as specific static methods like Bytes.fromByteArray and ByteArray.fromBigInt to facilitate conversions [6][7]. Usage: It is generally recommended to use the Bytes type for entity IDs and Ethereum data types [5]. In the source code, Bytes is defined simply as an extension of ByteArray [4]: export class Bytes extends ByteArray {} For most subgraph development, Bytes is the preferred class to use unless a specific API requirement calls for a ByteArray [5].
Citations:
- 1: https://thegraph.com/docs/en/subgraphs/developing/creating/graph-ts/api/
- 2: https://thegraph.academy/docs-sub/official-docs/developer/assemblyscript-api/
- 3: https://github.com/graphprotocol/docs/blob/main/website/src/pages/en/subgraphs/developing/creating/graph-ts/api.mdx
- 4: https://github.com/graphprotocol/graph-ts/blob/73054c382676f5e9723c52845fe730fd954baacd/index.ts
- 5: https://thegraph.com/docs/en/subgraphs/developing/creating/assemblyscript-mappings/
- 6: https://thegraph.com/docs/en/resources/migration-guides/assemblyscript-migration-guide/
- 7: https://github.com/TheGraphAcademy/Graph-Academy-Hub/blob/main/official-docs/developer/assemblyscript-migration-guide.md
Return a Bytes value from the legacy padding helper.
new Bytes(12).concat(address) returns ByteArray, and AssemblyScript does not treat that ByteArray result as the Bytes return type. Use a Bytes-typed pad before returning it.
Proposed fix
function left_padAddressTo32Bytes(address: Bytes): Bytes {
- return new Bytes(12).concat(address)
+ let padded = new Bytes(32)
+ for (let i = 0; i < 20; i++) {
+ padded[i + 12] = address[i]
+ }
+ return padded
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mappingRoutedDeposit.ts` around lines 30 - 32, Update
leftPadAddressTo32Bytes to use a Bytes-typed 12-byte padding value before
concatenating with address, ensuring the returned concatenation is accepted as
Bytes rather than ByteArray.
Adds server-side indexing of the routed (gasless / direct-mint) deposit
destination owner so a `deposits(where: { destinationOwner })` query can replace
the dapp's client-side ~45-95-request chunked eth_getLogs scan
(searchRoutedDepositIdsByDestination).
The five L1 depositor proxies (gasless, Arbitrum, Base, Sui, StarkNet) all
inherit AbstractL1BTCDepositor and emit:
DepositInitialized(uint256 indexed depositKey,
bytes32 indexed destinationChainDepositOwner,
address indexed l1Sender)
DepositFinalized(...same three indexed..., uint256 initialAmount, uint256 tbtcAmount)
depositKey is byte-identical to the Bridge deposit key = Deposit.id
(keccak256(fundingTxHash|fundingOutputIndex)), so the mapping attaches
destinationOwner straight onto the existing Deposit via getOrCreateDeposit — no
new entity or join. destinationChainDepositOwner is already the 32-byte form the
dapp's normalizeDestinationTopic() searches on (EVM address left-padded;
Sui/StarkNet raw bytes32), so the lookup is a drop-in.
- schema.graphql: Deposit gains destinationOwner, l1Sender, isRouted.
- abis/RoutedDepositor.json: the two events (all three leading params indexed).
- subgraph.yaml: five ethereum datasources (startBlock 20,650,000).
- src/mappingRoutedDeposit.ts: attaches destinationOwner/l1Sender/isRouted.
Topic0s verified via keccak256: e96a7294=DepositInitialized(uint256,bytes32,address),
3ce59c3f=DepositFinalized(...). `graph codegen` + `graph build` pass.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The L1 depositors emitted a legacy DepositInitialized/DepositFinalized overload (owner as an indexed address) before switching to the current bytes32 form, and the legacy era covers roughly 87-90% of Arbitrum/Base routed deposits. Index both overloads on every depositor datasource and left-pad legacy owners to the same 32-byte destinationOwner form the current events store, so one where-filter matches both eras. Also: - set each datasource startBlock to the contract's real deploy block (the Arbitrum depositor deployed at 20,632,547, below the previous 20,650,000 floor) and register all five depositors in networks.json for mainnet and sepolia so graph build --network keeps working - keep the l1Sender recorded by the first routed event seen instead of letting a permissionless finalization overwrite the initializer - reuse Utils.convertDepositKeyToHex for the deposit id conversion
getOrCreateDeposit would save a Deposit missing non-nullable fields if a routed event ever arrived before the Bridge's DepositRevealed, and a failed save halts the entire subgraph. The same-transaction ordering makes that unreachable today, but load-and-skip with a warning is strictly safer insurance ahead of a multi-day resync.
…Depositors After rebase onto master (which introduced L1BTCRedeemerWormhole via PR threshold-network#3), the conflict resolution left L1BTCRedeemerWormhole's mapping block nested inside the GaslessDepositor data source, causing a duplicate-key YAML parse error. Restructure: L1BTCRedeemerWormhole now has its own complete data source block, followed by the 5 RoutedDepositor data sources from PR threshold-network#7.
eedebe1 to
d77e39d
Compare
90d4188
into
threshold-network:master
2 participants
Why
The tBTC dapp resolves and searches routed (gasless / direct-mint) deposits by their destination-chain owner. Because the subgraph doesn't index that owner today, the frontend does it on-chain:
searchRoutedDepositIdsByDestinationruns a ~45–95-request chunkedeth_getLogsscan (5 contracts, 3 topics) for every destination search. That's slow, hammers the client RPC key, and can't be cached well.This indexes the destination owner so the whole scan collapses into one
deposits(where: { destinationOwner })query.How
The five L1 depositor proxies — gasless, Arbitrum, Base, Sui, StarkNet — all inherit
AbstractL1BTCDepositor. Both event eras are indexed:The legacy era is not an edge case — it's the majority of Arbitrum/Base routed history (Arbitrum: 723 of 833 initializations; Base: 303 of 338; counted on-chain). Those deposits were initialized and finalized under the legacy implementation, so indexing only the bytes32 variant would silently drop ~87–90% of Arbitrum/Base routed deposits from destination search. The legacy handlers left-pad the 20-byte address owner to the same 32-byte form the bytes32 era stores, so one
where: { destinationOwner }filter matches both eras.The key enabling fact:
depositKey(uint256) is byte-identical to the Bridge deposit key, which is alreadyDeposit.id(keccak256(fundingTxHash | fundingOutputIndex)). The mapping attaches the owner onto the existingDeposit— no new entity, no join (id conversion reuses the repo's provenconvertDepositKeyToHex). It loads-and-skips (with a warning) rather than get-or-creates: if a routed event ever arrived for an unknown key, creating a Deposit missing non-nullable fields would halt the whole subgraph, so dropping one attachment is strictly safer.Changes
schema.graphql—DepositgainsdestinationOwner: Bytes,l1Sender: Bytes(the sender of the first routed event seen — a later permissionlessfinalizeDepositno longer overwrites the initializer),isRouted: Boolean.abis/RoutedDepositor.json— all four events (both eras).subgraph.yaml— fiveethereumdatasources with per-contract deploy blocks (gasless 23,576,205 · arbitrum 20,632,547 · base 21,961,116 · sui 22,842,726 · starknet 22,671,770), four event handlers each. The Arbitrum block fixes a real gap — it deployed 17,453 blocks below the previous uniform 20,650,000 floor.networks.json— the five datasources added for mainnet (same deploy blocks) and sepolia (real testnet deployments from the tbtc-v2 artifacts, cross-checked on-chain), soyarn build-mainnet/yarn build-sepoliawork —graph build --networkhard-fails on any manifest datasource missing from the network config, which the previous revision hit.src/mappingRoutedDeposit.ts— handlers for both eras; documents the AssemblyScript 0.19 gotcha that== nullon a nullableBytesfield crashes the compiler (truthiness check used instead).Verification
DepositInitializedhash byte-matches the third topic in the dapp'sROUTED_DEPOSIT_EVENT_TOPICSscan list.graph codegen,graph build,yarn build-mainnet, andyarn build-sepoliaall pass on this branch. Note for maintainers:graph build --network …rewritessubgraph.yamlin place (drops comments / swaps addresses) — that mutation is a build side effect, don't commit it.Drop-in client query (follow-up in the dapp)
$owneris exactlynormalizeDestinationTopic(searchQuery)— valid for both event eras thanks to the left-padding;deposit.idequals the oldtopics[1], sosearchRoutedDepositIdsByDestinationbecomesdeposits.map(d => d.id). (I'll open that dapp PR once this is deployed — it can't return results before the reindex completes.)Deploy notes — maintainer / Threshold action (I can't do these)
graftat head would skip pre-graft routed history, so a full resync is preferable for backfill.)yarn build-mainnet && yarn deploy-mainnet→ bump version → publish). Version updates keep the same subgraph id, so theapi.threshold.networkproxy's gateway secret needs no change.Summary by CodeRabbit