feat: channel upgrade #56
Conversation
WalkthroughAdds end-to-end IBC channel upgrade support: migration and table, DB controller, event parsing, REST endpoints and proof helpers, worker and wallet integration for upgrade message generation and timeout handling, type and message-surface updates, a dependency version bump, and README formatting/spec additions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant SrcChain as Source Chain
participant Relayer as Rapid Relayer
participant DB as DB (channel_upgrade)
participant DstChain as Destination Chain
note over SrcChain,DstChain: Channel Upgrade Lifecycle (high level)
SrcChain->>Relayer: Events: channel_upgrade_init/try/ack/confirm/open/error
Relayer->>Relayer: parseChannelUpgradeEvent()
Relayer->>DB: ChannelUpgradeController.feedEvents(...) (deferred ops)
Relayer->>DB: Tx commit: insert/update/cleanup records
loop Worker cycle
Relayer->>DstChain: REST: channel/upgrade or upgrade_error (as needed)
Relayer->>Relayer: getChannelProof / getUpgradeProof / getUpgradeErrorProof
Relayer->>DstChain: Tx: MsgChannelUpgradeTry/Ack/Confirm/Open/Timeout/Cancel
DstChain-->>Relayer: Result (success/error)
Relayer->>DB: update in_progress / delete on completion or error
end
sequenceDiagram
autonumber
participant ChainWorker as ChainWorker
participant WalletWorker as WalletWorker
participant DB as DB
participant REST as REST (IBC API)
participant Proofs as Proof Utils
ChainWorker->>ChainWorker: fetchEvents(height > 0)
ChainWorker->>ChainWorker: parseChannelUpgradeEvent(...)
ChainWorker->>DB: feedEvents(...), in tx with other feeds
WalletWorker->>DB: getChannelUpgradeEvents(filter/state)
WalletWorker->>WalletWorker: filterChannelUpgradeEvents()
alt Timeout check
WalletWorker->>REST: getUpgrade()/getChannel()
WalletWorker->>WalletWorker: checkChannelUpgradeTimeout()
WalletWorker->>DB: mark UPGRADE_TIMEOUT (if expired)
end
WalletWorker->>Proofs: getChannelProof()/getUpgradeProof()/getUpgradeErrorProof()
WalletWorker->>REST: get counterparty channel/upgrade (as needed)
WalletWorker->>ChainWorker: build and send upgrade Msg*
ChainWorker-->>WalletWorker: tx result
WalletWorker->>DB: delete/cleanup or clear in_progress
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
README.md (1)
53-55: Port out of valid range (0–65535) in JSON example
"metricPort": 70001exceeds the valid TCP/UDP port range; users copying this will fail at startup.Apply this diff to present a valid port (example: 7001):
- "metricPort": 70001, + "metricPort": 7001,src/db/utils.ts (2)
227-241: Bug:order()usesObject.keys(where)instead oforderThis prevents ORDER BY from working and can produce empty/incorrect SQL. High user-visible impact for any caller relying on sorting.
Apply this diff:
function order<T>(order: Order<T>): string { let sql = '' const orders: string[] = [] - const keys = Object.keys(where) as (keyof T)[] + const keys = Object.keys(order) as (keyof T)[] for (const key of keys) { orders.push(`${String(key)} ${order[key]}`) } if (orders.length !== 0) { sql = ` ORDER BY ${orders.join(', ')} ` } return sql }
143-162: Ensure consistent SQL bindings inupdate
Theupdatefunction currently mixes named placeholders ($key) for the SET clause with positional placeholders (?) in the WHERE clause, then calls.run(params, set). better-sqlite3 does not merge an array and an object into one binding context—this will lead to unbound parameters or misbinding at runtime. Refactor to use a single placeholder style:• Location:
src/db/utils.ts, functionupdate(lines ~143–162)• Replace named SET placeholders and dual-argument
.runwith pure positional parameters:export function update<T>( db: Database, tableName: string, set: Partial<T>, wheres?: WhereOptions<T>[] ) { - let sql = `UPDATE ${tableName} SET` - const params = [] - const keys = Object.keys(set as object) - const placeHolder = keys.map((key) => ` ${key} = $${key}`).join(',') - sql += placeHolder + let sql = `UPDATE ${tableName} SET` + const keys = Object.keys(set as object) + const setPlaceholders = keys.map((k) => ` ${k} = ?`).join(',') + sql += setPlaceholders + + // Build positional params: SET values then WHERE params + const params: ParamType[] = keys.map((k) => + toParamType((set as Record<string, unknown>)[k]) + ) if (wheres) { const [whereSql, whereParams] = where(wheres) sql += whereSql params.push(...whereParams) } - db.prepare<unknown[], T>(sql).run(params, set) + db.prepare<unknown[], T>(sql).run(params) }• Alternatively, convert the WHERE clause to named placeholders (e.g.
$w0, $w1, …) and call.run({ …set, ...whereBindings })so all bindings use the same syntax.src/db/migration/index.ts (1)
31-36: Numeric sort bug:.sort()on numbers defaults to lexicographic ordering.
Array.prototype.sort()without a comparator coerces to strings, so versions like[1, 2, 10]will sort as[1, 10, 2]. This can misorder migrations.Apply:
- const unappliedVersions = migrations - .map(numberize) - .sort() + const unappliedVersions = migrations + .map(numberize) + .sort((a, b) => a - b)src/lib/eventParser.ts (1)
88-108: Bug:isSrcis always truthy for channel close events.
const isSrc = event.type === 'channel_close_init' || 'channel_close'always evaluates to true because the non-empty string is truthy. This flips src/dst mapping.Apply:
- const isSrc = event.type === 'channel_close_init' || 'channel_close' + const isSrc = + event.type === 'channel_close_init' || + event.type === 'channel_close_confirm'
🧹 Nitpick comments (27)
README.md (1)
79-79: Typo in example config key: "conneciton-1" → "connection-1"Minor, but copy-paste will propagate the typo.
- "connections": [{ "connectionId": "conneciton-1" }] + "connections": [{ "connectionId": "connection-1" }]src/db/utils.ts (2)
182-206: Guard against empty range objects to avoid invalid SQL "()"If a caller passes a Range object with none of gt/gte/lt/lte set (e.g., dynamic construction),
rangeConditionsstays empty and the code pushes"()", yielding a syntax error.Apply this diff:
- condition.push(`(${rangeConditions.join(' AND ')})`) + if (rangeConditions.length > 0) { + condition.push(`(${rangeConditions.join(' AND ')})`) + }Optionally assert at runtime when a Range object is empty to help callsites fail fast.
206-217: Direct injection viacustomwhere-clause string
customis concatenated directly into SQL. If any part of it is derived from external input, this is an injection vector.
- Prefer parameterized fragments (accept
{ custom: { sql: 'x = ?', params: [val] } }).- Or restrict
customusage to internal, static fragments and document it accordingly.I can draft a typed
SqlFragmenthelper if you want.src/lib/proof.ts (1)
62-90: Consolidate proof helpers into a single utilityThe IAVL key prefixes (
channelEnds/ports/...,channelUpgrades/upgrades/...,channelUpgrades/upgradeError/...) have been confirmed in src/lib/proof.ts (lines 54, 69, 84). To reduce duplication and minimize drift, extract the shared proof-generation steps into one helper.• File: src/lib/proof.ts
– Proof functions at lines 54–90 (getChannelProof,getUpgradeProof,getUpgradeErrorProof)Example refactor:
+ function makeIbcKey(path: string): Uint8Array { + return new Uint8Array(Buffer.from(path)) + } + + async function getKeyedProof( + chain: ChainWorker, + keyPath: string, + headerHeight: Height + ): Promise<string> { + const proof = await getRawProof(chain, makeIbcKey(keyPath), headerHeight) + return Buffer.from(convertProofsToIcs23(proof)).toString('base64') + } @@ -54,12 +59,7 @@ export async function getChannelProof( portId: string, channelId: string, headerHeight: Height ): Promise<string> { - const key = new Uint8Array( - Buffer.from(`channelEnds/ports/${portId}/channels/${channelId}`) - ) - const proof = await getRawProof(chain, key, headerHeight) - const ics23Proof = convertProofsToIcs23(proof) - - return Buffer.from(ics23Proof).toString('base64') + return getKeyedProof( + chain, + `channelEnds/ports/${portId}/channels/${channelId}`, + headerHeight + ) } @@ -67,7 +67,7 @@ export async function getUpgradeProof( ): Promise<string> { - const key = new Uint8Array(Buffer.from( - `channelUpgrades/upgrades/ports/${portId}/channels/${channelId}` - )) - const proof = await getRawProof(chain, key, headerHeight) - const ics23Proof = convertProofsToIcs23(proof) - - return Buffer.from(ics23Proof).toString('base64') + return getKeyedProof( + chain, + `channelUpgrades/upgrades/ports/${portId}/channels/${channelId}`, + headerHeight + ) } @@ -80,7 +80,7 @@ export async function getUpgradeErrorProof( ): Promise<string> { - const key = new Uint8Array(Buffer.from( - `channelUpgrades/upgradeError/ports/${portId}/channels/${channelId}` - )) - const proof = await getRawProof(chain, key, headerHeight) - const ics23Proof = convertProofsToIcs23(proof) - - return Buffer.from(ics23Proof).toString('base64') + return getKeyedProof( + chain, + `channelUpgrades/upgradeError/ports/${portId}/channels/${channelId}`, + headerHeight + ) }This preserves the existing, verified key paths while centralizing the common logic.
src/lib/restClient.ts (1)
149-166: Type consistency of proof heights
ChannelResponseRaw.proof_heightusesnumbertypes while other endpoints (e.g.,NextSequence,ConsState) usestring. This mirrors many Cosmos REST APIs, but downstream code should not assume a single shape.Consider normalizing to
Longorstringat the boundary for consistency. I can provide a thin converter if needed.src/db/migration/0.0.5.sql (1)
8-8: Clarify the state enum comment to include TIMEOUT rationale.Your spec enumerates
UPGRADE_TIMEOUT = 10, while the schema comment only lists up to 9. If timeouts are intentionally not persisted (as the spec states), note that in the comment to avoid confusion for maintainers.Suggested tweak:
- state INTEGER NOT NULL, -- State enum: UPGRADE_TRY=5, UPGRADE_ACK=6, UPGRADE_CONFIRM=7, UPGRADE_OPEN=8, UPGRADE_ERROR=9 + state INTEGER NOT NULL, -- State enum: UPGRADE_TRY=5, UPGRADE_ACK=6, UPGRADE_CONFIRM=7, UPGRADE_OPEN=8, UPGRADE_ERROR=9 + -- NOTE: UPGRADE_TIMEOUT=10 is handled in-memory and is not persisted.spec/IBC_CHANNEL_UPGRADE_IMPLEMENTATION.md (5)
21-41: Document all columns that exist in the migration (missing upgrade_connection_id fields).The migration defines
upgrade_connection_idandcounterparty_upgrade_connection_id, but the “Fields” list in this section doesn’t mention them. Add both fields (and their purpose) to keep the spec aligned with the schema.
41-41: Call out non-persistence of TIMEOUT in both spec and schema.You state timeouts aren’t stored. Add an explicit note that
UPGRADE_TIMEOUT=10is not persisted to thechannel_upgradetable to align expectations with the enum values used elsewhere.
102-109: Event attribute key for connection hops: verify actual key emitted by ibc-go.The text mentions ibc-go emits
ConnectionHops[0]as a single string, but the parser uses'connection_hops'. Please double-check the exact attribute name(s) in your target ibc-go version and document all supported variants (e.g.,connection_hops,connection_hops[0]). This avoids silent parsing failures across versions.I can adjust the parser to probe multiple candidate keys and normalize the value.
153-156: “UPGRADE_INIT” appears in flow but isn’t a persisted state.The state progression shows “UPGRADE_INIT → …” but the enum doesn’t include INIT and the DB stores from TRY onward. Suggest clarifying that INIT is an event only (no DB row) to avoid confusion.
153-153: Typo: code fence language should be plaintext.Minor formatting issue.
-```plantext +```plaintextsrc/db/migration/index.ts (1)
48-48: Ensure reset is idempotent and efficient.Please confirm
ChannelUpgradeController.resetUpgradeInProgress(db)only flipsin_progressfrom 1→0 without scanning the entire table repeatedly, and that it’s safe to run when the new table doesn’t exist (on older versions).If helpful, I can add a guard like
IF EXISTS(channel_upgrade)in SQL or a try/catch that ignores “no such table”.src/lib/eventParser.ts (1)
29-35: Optional: guard against missing timeout fields in packet events.If
packet_timeout_heightorpacket_timeout_timestampis absent, the current parsing producesNaN. Consider defaulting toundefinedto avoid downstream arithmetic onNaN.I can propose a small patch if you want this tightened now.
src/types/table.ts (1)
154-159: Consider documenting the upgrade state machine transitionsThe new upgrade states (UPGRADE_TRY through UPGRADE_TIMEOUT) should have their valid state transitions documented. This would help prevent invalid state transitions in the implementation.
Consider adding JSDoc comments to document valid state transitions:
export enum ChannelState { INIT = 1, TRYOPEN = 2, ACK = 3, CLOSE = 4, + /** Initiates upgrade handshake on counterparty chain */ UPGRADE_TRY = 5, + /** Acknowledges counterparty's upgrade proposal */ UPGRADE_ACK = 6, + /** Confirms upgrade parameters are accepted */ UPGRADE_CONFIRM = 7, + /** Opens channel with upgraded parameters */ UPGRADE_OPEN = 8, + /** Indicates upgrade failed and needs rollback */ UPGRADE_ERROR = 9, + /** Indicates upgrade timed out */ UPGRADE_TIMEOUT = 10, }src/types/event.ts (1)
88-100: Consider making channelState field requiredThe
channelStatefield is marked as optional but seems essential for tracking upgrade progression. Unless there are specific scenarios where this field is intentionally absent, consider making it required for better type safety.export interface ChannelUpgradeInfo { height: number srcPortId: string srcChannelId: string dstPortId: string dstChannelId: string upgradeSequence: number upgradeVersion?: string upgradeOrdering?: string upgradeConnectionHops?: string upgradeErrorReceipt?: string - channelState?: string + channelState: string }src/workers/wallet.ts (1)
199-217: Consider extracting timeout check logicThe timeout check is performed inline within the Promise.all mapping. Consider extracting this into a separate method for better testability and readability.
private async processUpgradeTimeouts( channelUpgradeEvents: ChannelUpgradeTable[] ): Promise<void> { await Promise.all( channelUpgradeEvents.map(async (event) => { if ( event.state !== ChannelState.UPGRADE_ACK && event.state !== ChannelState.UPGRADE_CONFIRM ) { return } const isTimeout = await checkChannelUpgradeTimeout( event, this.chain, this.workerController.chains[event.counterparty_chain_id] ) if (isTimeout) { event.state = ChannelState.UPGRADE_TIMEOUT } }) ) }src/db/controller/channelUpgrade.ts (6)
64-74: Defensive checks and clearer error messages for origin connection lookup.If REST returns no channel or no connection_hops, this will throw ambiguously or cause undefined access. Add guards and explicit errors to aid ops.
Example:
- const connection_id = channel.channel.connection_hops[0] + const hops = channel?.channel?.connection_hops + if (!hops?.length) { + throw new Error( + `origin connection_hops missing for ${event.channelUpgradeInfo.srcPortId}/${event.channelUpgradeInfo.srcChannelId} on ${chainId}` + ) + } + const connection_id = hops[0]
288-299: Placeholders for connection_id may break filtering — verify worker fills before querying.UPGRADE_OPEN records store 'placeholder' in connection_id/counterparty_connection_id. However, getChannelUpgradeEvents filters by connection_id when PacketFilter.connections is provided. If the wallet’s filter relies on connection_id, OPEN events with placeholders won’t be discoverable.
Consider one of:
- Update getChannelUpgradeEvents to include upgrade_connection_id when state === UPGRADE_OPEN (OR condition).
- Ensure the wallet overwrites placeholders before calling getChannelUpgradeEvents.
- Store a sentinel value and explicitly ignore connection filters for UPGRADE_OPEN.
If you want, I can patch getChannelUpgradeEvents to add an alternate predicate for UPGRADE_OPEN.
Also applies to: 394-431
354-356: Comment contradicts behavior.“You create error state only if the counterparty channel is not open” but the code unconditionally inserts. Either implement the guard or fix the comment.
If implementing the guard here is impractical due to REST client scope, update the comment to avoid confusion.
314-316: Comment contradicts behavior (same as above).The code unconditionally inserts the OPEN state; comment claims it’s conditional.
Align the comment with actual behavior or add the guard.
404-422: Be explicit when state is optional — avoid passing undefined into where clauses.If select() includes undefined fields verbatim, these predicates can break filtering. Build where-objects conditionally.
Sketch:
const base = { in_progress: Bool.FALSE, chain_id: chainId, counterparty_chain_id: In(counterpartyChainIds), } as Partial<ChannelUpgradeTable> if (state !== undefined) base.state = state wheres.push(base as WhereOptions<ChannelUpgradeTable>)Also applies to: 424-431
122-125: Log volume: stringify full events may bloat logs.Consider lowering to debug or logging summarized identifiers (port/channel/seq). This reduces noise in busy environments.
src/workers/index.ts (5)
357-363: Safer mapping for upgrade_ordering.Hardcoding ORDER_ORDERED vs ORDER_UNORDERED ignores ORDER_NONE_UNSPECIFIED and risks typos. Map via the enum.
Apply this diff:
- const counterpartyUpgradeFields = new UpgradeFields( - event.upgrade_ordering === 'ORDER_ORDERED' - ? Order.ORDER_ORDERED - : Order.ORDER_UNORDERED, + const ordering = + Order[event.upgrade_ordering as keyof typeof Order] ?? + Order.ORDER_UNORDERED + const counterpartyUpgradeFields = new UpgradeFields( + ordering, [event.counterparty_upgrade_connection_id as string], event.upgrade_version || '' )
349-390: Validate presence of connection hop IDs before constructing messages.Both upgrade_connection_id and counterparty_upgrade_connection_id are cast with “as string”. If absent, you’ll embed undefined in the hops arrays.
Guard early:
if (!event.upgrade_connection_id || !event.counterparty_upgrade_connection_id) { throw new Error(`missing upgrade connection hops for ${event.port_id}/${event.channel_id}`) }If you prefer soft-fail, return undefined and let the caller skip.
433-479: Confirm path mirrors ACK — same undefined handling and proof resolution.Looks consistent; just ensure undefined is handled by callers as above.
512-541: Timeout path: correct inclusion of full Channel on counterparty.Using Channel.fromData is appropriate. Consider validating channel existence and state if you want better error messages.
53-65: Minor: imports could use orderFromJSON for ordering (optional).If available in your proto package, orderFromJSON would be clearer than enum indexing. Not required if you adopt the enum index approach suggested above.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (17)
README.md(1 hunks)package.json(1 hunks)spec/IBC_CHANNEL_UPGRADE_IMPLEMENTATION.md(1 hunks)src/db/controller/channelUpgrade.ts(1 hunks)src/db/migration/0.0.5.sql(1 hunks)src/db/migration/index.ts(2 hunks)src/db/utils.ts(1 hunks)src/lib/eventParser.ts(2 hunks)src/lib/proof.ts(1 hunks)src/lib/restClient.ts(4 hunks)src/msgs/channelOpenTry.ts(2 hunks)src/msgs/index.ts(1 hunks)src/types/event.ts(1 hunks)src/types/table.ts(2 hunks)src/workers/chain.ts(9 hunks)src/workers/index.ts(5 hunks)src/workers/wallet.ts(18 hunks)
🧰 Additional context used
🪛 LanguageTool
spec/IBC_CHANNEL_UPGRADE_IMPLEMENTATION.md
[grammar] ~23-~23: There might be a mistake here.
Context: ...### Migration (0.0.5.sql) - Table: channel_upgrade - Fields: - id: Primary key (auto-in...
(QB_NEW_EN)
[grammar] ~24-~24: There might be a mistake here.
Context: ...Table**: channel_upgrade - Fields: - id: Primary key (auto-increment) - `in_p...
(QB_NEW_EN)
[grammar] ~25-~25: There might be a mistake here.
Context: ...: - id: Primary key (auto-increment) - in_progress: Boolean flag for transaction status ...
(QB_NEW_EN)
[grammar] ~26-~26: There might be a mistake here.
Context: ...ss: Boolean flag for transaction status - state: Channel upgrade state (5-10) - chai...
(QB_NEW_EN)
[grammar] ~27-~27: There might be a mistake here.
Context: ... - state: Channel upgrade state (5-10) - chain_id: Destination chain ID (where the record...
(QB_NEW_EN)
[grammar] ~28-~28: There might be a mistake here.
Context: ... ID (where the record will be processed) - connection_id: Connection ID on destination chain -...
(QB_NEW_EN)
[grammar] ~29-~29: There might be a mistake here.
Context: ..._id: Connection ID on destination chain - port_id: Port ID on destination chain - chan...
(QB_NEW_EN)
[grammar] ~30-~30: There might be a mistake here.
Context: ... port_id: Port ID on destination chain - channel_id: Channel ID on destination chain - `c...
(QB_NEW_EN)
[grammar] ~31-~31: There might be a mistake here.
Context: ...nel_id: Channel ID on destination chain - counterparty_chain_id: Source chain ID - counterparty_conn...
(QB_NEW_EN)
[grammar] ~32-~32: There might be a mistake here.
Context: ...counterparty_chain_id: Source chain ID - counterparty_connection_id: Connection ID on source chain - `cou...
(QB_NEW_EN)
[grammar] ~33-~33: There might be a mistake here.
Context: ...ction_id: Connection ID on source chain - counterparty_port_id: Port ID on source chain - counterpa...
(QB_NEW_EN)
[grammar] ~34-~34: There might be a mistake here.
Context: ...rparty_port_id: Port ID on source chain - counterparty_channel_id: Channel ID on source chain - upgrad...
(QB_NEW_EN)
[grammar] ~35-~35: There might be a mistake here.
Context: ..._channel_id: Channel ID on source chain - upgrade_sequence: Upgrade sequence number - upgrade_v...
(QB_NEW_EN)
[grammar] ~36-~36: There might be a mistake here.
Context: ...grade_sequence: Upgrade sequence number - upgrade_version: New channel version - upgrade_order...
(QB_NEW_EN)
[grammar] ~37-~37: There might be a mistake here.
Context: ...- upgrade_version: New channel version - upgrade_ordering: Channel ordering (ORDERED/UNORDERED) ...
(QB_NEW_EN)
[grammar] ~38-~38: There might be a mistake here.
Context: ...g: Channel ordering (ORDERED/UNORDERED) - upgrade_error_receipt`: Error receipt for failed upgrades (nul...
(QB_NEW_EN)
[grammar] ~104-~104: There might be a mistake here.
Context: ... ConnectionHops[0] as a single string) - Uses version and ordering attributes...
(QB_NEW_EN)
[grammar] ~105-~105: There might be a mistake here.
Context: ...rdering` attributes directly from events - Handles different event types appropriat...
(QB_NEW_EN)
[grammar] ~106-~106: There might be a mistake here.
Context: ...dles different event types appropriately - Maps port/channel IDs correctly for sour...
(QB_NEW_EN)
[grammar] ~115-~115: There might be a mistake here.
Context: ...hannel Upgrade Init Event - port_id, channel_id - counterparty_port_id, counterparty_channel_id - `connectio...
(QB_NEW_EN)
[grammar] ~116-~116: There might be a mistake here.
Context: ... channel_id - counterparty_port_id, counterparty_channel_id - connection_hops, version, ordering - `upgrade_seque...
(QB_NEW_EN)
[grammar] ~122-~122: There might be a mistake here.
Context: ...Channel Upgrade Try Event - port_id, channel_id - counterparty_port_id, counterparty_channel_id - `connectio...
(QB_NEW_EN)
[grammar] ~123-~123: There might be a mistake here.
Context: ... channel_id - counterparty_port_id, counterparty_channel_id - connection_hops, version, ordering - `upgrade_seque...
(QB_NEW_EN)
[grammar] ~129-~129: There might be a mistake here.
Context: ...Channel Upgrade Ack Event - port_id, channel_id - counterparty_port_id, counterparty_channel_id - `connectio...
(QB_NEW_EN)
[grammar] ~130-~130: There might be a mistake here.
Context: ... channel_id - counterparty_port_id, counterparty_channel_id - connection_hops, version, ordering - `upgrade_seque...
(QB_NEW_EN)
[grammar] ~136-~136: There might be a mistake here.
Context: ...nel Upgrade Confirm Event - port_id, channel_id - counterparty_port_id, counterparty_channel_id - `channel_s...
(QB_NEW_EN)
[grammar] ~137-~137: There might be a mistake here.
Context: ... channel_id - counterparty_port_id, counterparty_channel_id - channel_state, upgrade_sequence ### 5. Controller ...
(QB_NEW_EN)
[grammar] ~167-~167: There might be a mistake here.
Context: ...fields** in database (avoids stale data) - Real-time timeout checking via REST AP...
(QB_NEW_EN)
[grammar] ~168-~168: There might be a mistake here.
Context: ...REST API queries (rest.ibc.getUpgrade) - Dynamic state updates based on current...
(QB_NEW_EN)
[grammar] ~169-~169: There might be a mistake here.
Context: ...ates** based on current chain conditions - Timeout comparison against current cha...
(QB_NEW_EN)
[grammar] ~243-~243: There might be a mistake here.
Context: ...elUpgradeTry** - For UPGRADE_TRY state 2. MsgChannelUpgradeAck - For `UPGRADE_AC...
(QB_NEW_EN)
[grammar] ~244-~244: There might be a mistake here.
Context: ...elUpgradeAck** - For UPGRADE_ACK state 3. MsgChannelUpgradeConfirm - For `UPGRAD...
(QB_NEW_EN)
[grammar] ~245-~245: There might be a mistake here.
Context: ...eConfirm** - For UPGRADE_CONFIRM state 4. MsgChannelUpgradeOpen - For `UPGRADE_O...
(QB_NEW_EN)
[grammar] ~246-~246: There might be a mistake here.
Context: ...UpgradeOpen** - For UPGRADE_OPEN state 5. MsgChannelUpgradeTimeout - For `UPGRAD...
(QB_NEW_EN)
[grammar] ~247-~247: There might be a mistake here.
Context: ...eTimeout** - For UPGRADE_TIMEOUT state 6. MsgChannelUpgradeCancel - For `UPGRADE...
(QB_NEW_EN)
[grammar] ~311-~311: There might be a mistake here.
Context: ...in B** - Database record is stored with: - chain_id: Chain B (destination) - `counterpart...
(QB_NEW_EN)
[grammar] ~312-~312: There might be a mistake here.
Context: ...h: - chain_id: Chain B (destination) - counterparty_chain_id: Chain A (source) - state: `UPGRADE...
(QB_NEW_EN)
[grammar] ~313-~313: There might be a mistake here.
Context: ...counterparty_chain_id: Chain A (source) - state: UPGRADE_TRY -port_id, channel_...
(QB_NEW_EN)
[grammar] ~336-~336: There might be a mistake here.
Context: ...hain B - Database record is stored with: - chain_id: Chain A (source) - `counterparty_cha...
(QB_NEW_EN)
[grammar] ~337-~337: There might be a mistake here.
Context: ...d with: - chain_id: Chain A (source) - counterparty_chain_id: Chain B (destination) - state: `UP...
(QB_NEW_EN)
[grammar] ~338-~338: There might be a mistake here.
Context: ...erparty_chain_id: Chain B (destination) - state: UPGRADE_ACK -port_id, channel_...
(QB_NEW_EN)
[grammar] ~361-~361: There might be a mistake here.
Context: ...hain A - Database record is stored with: - chain_id: Chain B (destination) - `counterpart...
(QB_NEW_EN)
[grammar] ~362-~362: There might be a mistake here.
Context: ...h: - chain_id: Chain B (destination) - counterparty_chain_id: Chain A (source) - state: `UPGRADE...
(QB_NEW_EN)
[grammar] ~363-~363: There might be a mistake here.
Context: ...counterparty_chain_id: Chain A (source) - state: UPGRADE_CONFIRM -port_id, chan...
(QB_NEW_EN)
[grammar] ~386-~386: There might be a mistake here.
Context: ...hain B - Database record is stored with: - chain_id: Chain A (source) - `counterparty_cha...
(QB_NEW_EN)
[grammar] ~387-~387: There might be a mistake here.
Context: ...d with: - chain_id: Chain A (source) - counterparty_chain_id: Chain B (destination) - state: `UP...
(QB_NEW_EN)
[grammar] ~388-~388: There might be a mistake here.
Context: ...erparty_chain_id: Chain B (destination) - state: UPGRADE_OPEN -port_id, channel...
(QB_NEW_EN)
[grammar] ~412-~412: There might be a mistake here.
Context: ...chains - Database record is stored with: - chain_id: Chain B (destination) - `counterpart...
(QB_NEW_EN)
[grammar] ~413-~413: There might be a mistake here.
Context: ...h: - chain_id: Chain B (destination) - counterparty_chain_id: Chain A (source) - state: `UPGRADE...
(QB_NEW_EN)
[grammar] ~414-~414: There might be a mistake here.
Context: ...counterparty_chain_id: Chain A (source) - state: UPGRADE_OPEN -port_id, channel...
(QB_NEW_EN)
[grammar] ~415-~415: There might be a mistake here.
Context: ...hain_id: Chain A (source) - state: UPGRADE_OPEN -port_id, channel_id`: Chain B's port/channel ...
(QB_NEW_EN)
[grammar] ~416-~416: There might be a mistake here.
Context: ...d, channel_id: Chain B's port/channel - counterparty_port_id, counterparty_channel_id`: Chain A's p...
(QB_NEW_EN)
[grammar] ~437-~437: There might be a mistake here.
Context: ...chains - Database record is stored with: - chain_id: Destination chain - `counterparty_ch...
(QB_NEW_EN)
[grammar] ~438-~438: There might be a mistake here.
Context: ... with: - chain_id: Destination chain - counterparty_chain_id: Source chain - state: `UPGRADE_ERR...
(QB_NEW_EN)
[grammar] ~439-~439: There might be a mistake here.
Context: ... - counterparty_chain_id: Source chain - state: UPGRADE_ERROR - `upgrade_error_rec...
(QB_NEW_EN)
[grammar] ~440-~440: There might be a mistake here.
Context: ...ty_chain_id: Source chain - state: UPGRADE_ERROR -upgrade_error_receipt`: Error details Cleanup: - All upg...
(QB_NEW_EN)
[grammar] ~454-~454: There might be a mistake here.
Context: ...E_CONFIRMstates only - Query REST API for current upgrade state:rest.ibc.getUpg...
(QB_NEW_EN)
[grammar] ~455-~455: There might be a mistake here.
Context: ..., channel_id)` - Compare timeout values with current chain state **Timeout Detectio...
(QB_NEW_EN)
[grammar] ~486-~486: There might be a mistake here.
Context: ...tics 1. Alternating Chain Processing: - Source chain → Destination chain → Sourc...
(QB_NEW_EN)
[grammar] ~487-~487: There might be a mistake here.
Context: ...chain → Source chain → Destination chain - Each chain processes events for the coun...
(QB_NEW_EN)
[grammar] ~491-~491: There might be a mistake here.
Context: ...→UPGRADE_ACK→UPGRADE_CONFIRM→UPGRADE_OPEN` - Each state is stored on the chain that w...
(QB_NEW_EN)
[grammar] ~494-~494: There might be a mistake here.
Context: ...ll process it next 3. Record Cleanup: - Previous state records are removed when ...
(QB_NEW_EN)
[grammar] ~496-~496: There might be a mistake here.
Context: ... - Prevents duplicate processing and maintains clean state 4. **Counterparty Perspect...
(QB_NEW_EN)
[grammar] ~498-~498: There might be a mistake here.
Context: ...an state 4. Counterparty Perspective: - Each database record represents what the...
(QB_NEW_EN)
[grammar] ~502-~502: There might be a mistake here.
Context: ...hain's perspective 5. Error Recovery: - Failed upgrades are cleaned up - Erro...
(QB_NEW_EN)
[grammar] ~507-~507: There might be a mistake here.
Context: ...other upgrades 6. Timeout Processing: - Limited to specific states (`UPGRADE_ACK...
(QB_NEW_EN)
[grammar] ~508-~508: There might be a mistake here.
Context: ...tates (UPGRADE_ACK, UPGRADE_CONFIRM) - In-memory state changes for immediate pr...
(QB_NEW_EN)
[grammar] ~509-~509: There might be a mistake here.
Context: ...y state changes for immediate processing - No database persistence required ## Usa...
(QB_NEW_EN)
[grammar] ~518-~518: There might be a mistake here.
Context: ...using correct attribute names 3. Parses and stores event data with proper structure...
(QB_NEW_EN)
[grammar] ~551-~551: There might be a mistake here.
Context: ...PGRADE_ACKandUPGRADE_CONFIRM` states - In-memory updates: State changes are m...
(QB_NEW_EN)
[grammar] ~552-~552: There might be a mistake here.
Context: ...ges are made in memory for immediate use - No database persistence: Timeout state...
(QB_NEW_EN)
[grammar] ~553-~553: There might be a mistake here.
Context: ...**: Timeout state changes are not saved to database ## Configuration ### Databas...
(QB_NEW_EN)
[grammar] ~600-~600: There might be a mistake here.
Context: ...*: Matches IBC-Go implementation exactly - Complete: Includes all upgrade message...
(QB_NEW_EN)
[grammar] ~601-~601: There might be a mistake here.
Context: ...des all upgrade message types and states - Robust: Handles errors gracefully and ...
(QB_NEW_EN)
[grammar] ~602-~602: There might be a mistake here.
Context: ...ors gracefully and maintains consistency - Efficient: Uses proper indexing, filte...
(QB_NEW_EN)
[grammar] ~603-~603: There might be a mistake here.
Context: ...iltering, and selective timeout checking - Maintainable: Follows established code...
(QB_NEW_EN)
🪛 Biome (2.1.2)
src/workers/wallet.ts
[error] 1100-1103: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1104-1108: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (22)
package.json (1)
21-21: Drop the leading “v” in the semver range
The version1.0.16exists on npm and resolves correctly without thevprefix (confirmed vianpm view @initia/initia.js versionand listed versions), so updating to a clean semver avoids potential tooling edge cases.• File:
package.json(line 21)
• Change:- "@initia/initia.js": "^v1.0.16", + "@initia/initia.js": "^1.0.16",README.md (1)
290-296: LGTM on heading normalizationRemoving trailing colons improves consistency. No functional impact.
src/db/utils.ts (1)
266-280: LGTM: ergonomic range helpersThese helpers align well with the
where()parser and improve callsite readability.src/lib/proof.ts (1)
14-15: The script above will dump the contents ofsrc/lib/transform.tsso we can confirm whetherrevisionHeightin our code is indeed a BigInt (as used inproof.ts), or if it’s still modeled as aLong/string (as in the originalcosmjs-types). Once we see that, we can determine if the subtraction with1nis valid or needs the saferLonghandling.src/lib/restClient.ts (1)
41-57: Verify enum JSON schema matches Channel.DataYour approach of mapping incoming wire strings to
State/Orderenums and then serializing them back to JSON viastateToJSON/orderToJSONis sound for normalizing unknown values toUNRECOGNIZED. Before approving, please confirm that the generatedChannel.Datatype expects string-based enum values (not numeric codes) for itsstateandorderingfields:
- Inspect the
Channel.Datadefinition in the generated TypeScript file under@initia/initia.proto/ibc/core/channel/v1/channel(e.g. in your localnode_modules) to verify that:
stateis typed as astring(matchingstateToJSONoutput)orderingis typed as astring(matchingorderToJSONoutput)- If
Channel.Datainstead expects numeric enum values, update the serializer to emit the numeric enums directly (e.g. omit*ToJSONcalls).Spot-check with a quick grep:
# In your project root (after installing deps) rg -nP --type ts 'interface Channel\.Data' -C3 node_modules/@initia/initia.proto/ibc/core/channel/v1/channel.tsOnce confirmed, we can safely approve these changes.
src/db/migration/index.ts (1)
6-6: Good: upgrade in-progress reset is now covered.Importing
ChannelUpgradeControllerhere keeps the reset logic consistent with packets/channels after migrations.src/msgs/index.ts (1)
2-9: Re-exports look good and broaden the public surface.The added exports for
channelCloseConfirmandsignedHeaderalign with the rest of the PR. No issues.src/msgs/channelOpenTry.ts (2)
1-10: LGTM! Import refactoring aligns with proto-based approachThe migration from
@initia/initia.jsto@initia/initia.protofor theStateimport and addition oforderFromJSONis appropriate for the proto-based channel state handling.
25-32: Manual verification required: ConfirmChannelconstructor signature in@initia/initia.jsThe
Channelclass is imported from the external package@initia/initia.js, so its constructor signature isn’t available in this repository. Please verify the following:
- That the constructor defined in
@initia/initia.jsaccepts six parameters, with the sixth being the upgrade sequence.- That using
0as the default upgrade sequence forchannelOpenTryaligns with the package’s expectations and won’t introduce unintended behavior.No changes to the diff are required here—just a manual confirmation against the external module.
src/types/table.ts (1)
106-127: LGTM! Well-structured channel upgrade table interfaceThe
ChannelUpgradeTableinterface is comprehensive and follows the existing table patterns. The fields appropriately capture all necessary upgrade state information including optional fields for progressive upgrade states.src/types/event.ts (1)
74-86: LGTM! Clean event type definitionThe
ChannelUpgradeEventinterface follows the existing event patterns and includes the essential upgrade event types. The commented-out event types suggest future extensibility has been considered.src/workers/chain.ts (3)
167-169: Good defensive programming with height validationAdding
height > 0check prevents processing invalid heights, which is a good defensive measure.
337-349: LGTM! Comprehensive upgrade event handlingThe channel upgrade event parsing covers all necessary upgrade event types and follows the existing event handling pattern consistently.
223-232: Transaction atomicity for channel upgrade events confirmedAll
feedEventscallbacks are purely synchronous and invoke BetterSQLite3’sdb.prepare().run()methods directly, so any error during an insert, delete, or update will throw and bubble out of the transaction callback, triggering an automatic rollback. No asynchronous operations occur inside the transaction, so the upgrade, open, and packet feeds execute as a single atomic unit. No changes are required.src/workers/wallet.ts (3)
135-136: Good fix for potential negative remain valuesUsing
Math.max(0, remain)prevents negative values which could cause unexpected behavior in subsequent logic.
1056-1070: Connection ID Remapping Documentation NeededI verified that there are no existing references to
UPGRADE_OPENor related connection remapping behavior in our Markdown spec files. Please ensure the upgrade specification explicitly documents this logic so future reviewers and implementers understand the need to use the original connection duringUPGRADE_OPEN.• Double-check the spec under
docs/upgrade.md(or equivalent) for an “Upgrade States” section and add a note about remapping to the original connection ID when handling theUPGRADE_OPENevent.
• If our spec lives elsewhere (e.g.仕様/upgrade_spec.txt), please update that file accordingly.
• Once added, link to the spec in code comments for direct traceability.
1152-1186: No overflow risk with timeout calculationThe
latestTimestampvalue is assigned viaDate.now()(ornew Date().valueOf()) and thus is a millisecond‐precision number (~1 × 10^12 today), well within JavaScript’s safe integer range (≤2^53–1). Converting that to aBigIntand multiplying by1_000_000to obtain nanoseconds remains safe, and JavaScript’sBigInttype supports arbitrarily large integers without overflow.• Location of assignment:
src/workers/chain.ts:this.latestTimestamp = Date.now() // … this.latestTimestamp = new Date().valueOf()• No changes required to the calculation itself.
• (Optional) You may apply the suggested diff to annotate units for clarity:- BigInt(chain.latestTimestamp) * BigInt(1000000) > timeout_timestamp + // latestTimestamp is in milliseconds, timeout_timestamp is in nanoseconds + BigInt(chain.latestTimestamp) * BigInt(1000000) > timeout_timestampsrc/db/controller/channelUpgrade.ts (1)
365-379: Bug: cleanUpgrade won’t remove paired rows reliably (matches counterparty_ instead of chain fields).*For both sides of a given upgrade_sequence you need to target the actual chain row fields (chain_id/port_id/channel_id), not the counterparty_* fields. As written, one or both sides may remain.
Apply this diff:
- del<ChannelUpgradeTable>(DB, ChannelUpgradeController.tableName, [ - { - counterparty_chain_id: counterpartyChainId, - counterparty_port_id: event.channelUpgradeInfo.dstPortId, - counterparty_channel_id: event.channelUpgradeInfo.dstChannelId, - upgrade_sequence: sequence - }, - { - counterparty_chain_id: chainId, - counterparty_port_id: event.channelUpgradeInfo.srcPortId, - counterparty_channel_id: event.channelUpgradeInfo.srcChannelId, - upgrade_sequence: sequence, - }, - ]) + del<ChannelUpgradeTable>(DB, ChannelUpgradeController.tableName, [ + { + chain_id: counterpartyChainId, + port_id: event.channelUpgradeInfo.dstPortId, + channel_id: event.channelUpgradeInfo.dstChannelId, + upgrade_sequence: sequence, + }, + { + chain_id: chainId, + port_id: event.channelUpgradeInfo.srcPortId, + channel_id: event.channelUpgradeInfo.srcChannelId, + upgrade_sequence: sequence, + }, + ])Likely an incorrect or invalid review comment.
src/workers/index.ts (4)
270-279: LGTM: correct use of stateFromJSON for TIMEOUT vs TIMEOUT_ON_CLOSE.Switching from string compare to enum parsing is safer and future-proof.
399-431: Ack path depends on counterparty getUpgrade; confirm that callers handle undefined.This may return undefined when upgrade info isn’t yet available. Ensure wallet/worker call sites branch appropriately to avoid enqueuing empty tx batches.
I can scan the repo to confirm all call sites handle undefined gracefully.
481-510: Open path uses only channel proof — is upgrade proof required by chain version?Some implementations require only channel proof at OPEN, others may also expect upgrade state proof. If your target chains expect more, add it here.
If needed, you can mirror the pattern from ACK/CONFIRM to fetch getUpgradeProof and include it in MsgChannelUpgradeOpen (if supported by the SDK).
543-575: Cancel path: undefined handling for errorReceipt + proof retrieval symmetry.Looks good. Same note as ACK: ensure callers handle undefined returns.
| - **Selective Timeout Checking**: Only processes timeouts for relevant states | ||
| - **In-Memory Updates**: Avoids unnecessary database writes for temporary state changes | ||
| - **Efficient Filtering**: Uses database queries to filter events before processing | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
“Uses proper indexing” claim mismatches current migration (no indexes created).
The migration doesn’t create any indexes yet. Either add the indexes (recommended) or tone down this claim in the doc.
See my schema comment for concrete index suggestions.
🤖 Prompt for AI Agents
In spec/IBC_CHANNEL_UPGRADE_IMPLEMENTATION.md around lines 584 to 587, the
document claims "Uses proper indexing" but the current migration creates no
indexes; either update the migration to add the recommended indexes (e.g., add
indexes on columns used in filters/joins such as channel_id, port_id,
block_height/timestamp, state/status) to match the claim, or remove/soften the
indexing claim in the doc to avoid inconsistency — follow the schema comments
for exact index names and columns, and ensure the doc and migration remain
consistent.
| return () => { | ||
| for (const fn of feedFns) { | ||
| fn() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Group DB mutations in a transaction to keep state consistent.
The returned closure iterates and mutates the DB without a transaction. A failure mid-way can leave the upgrade state inconsistent (e.g., deleted TRY but missing ACK). Wrap execution in a transaction.
Example:
- return () => {
- for (const fn of feedFns) {
- fn()
- }
- }
+ return () => {
+ DB.transaction(() => {
+ for (const fn of feedFns) fn()
+ })()
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/db/controller/channelUpgrade.ts around lines 52-57, the returned closure
currently iterates feedFns and performs DB mutations without a transaction;
update it to run all feedFns inside a single DB transaction: open a transaction
before looping, execute each fn using the transactional client/connection,
commit the transaction only if all fns succeed, and rollback the transaction if
any fn throws (then rethrow the error). Ensure the transactional client is used
by the feedFns or wrap each call so their DB operations use the same transaction
context, and handle errors to guarantee rollback on failure.
| const upgrade_connection_id = event.channelUpgradeInfo | ||
| .upgradeConnectionHops as string | ||
| const upgrade_connection = await ConnectionController.getConnection( | ||
| rest, | ||
| chainId, | ||
| upgrade_connection_id | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Robustly handle upgradeConnectionHops (likely array) — avoid as string casts.
Given the naming and parity with connection_hops, upgradeConnectionHops is likely string[]. Casting to string risks runtime errors. Safely extract the first hop.
Apply these diffs to use a helper:
- const upgrade_connection_id = event.channelUpgradeInfo
- .upgradeConnectionHops as string
+ const upgrade_connection_id = firstHopId(
+ event.channelUpgradeInfo.upgradeConnectionHops
+ )And add this utility near the top of the file (outside the shown ranges):
function firstHopId(hops?: string | string[] | null): string {
if (Array.isArray(hops)) return hops[0] ?? ''
return hops ?? ''
}Also applies to: 128-134, 178-184, 276-282
| return () => { | ||
| del<ChannelUpgradeTable>(DB, ChannelUpgradeController.tableName, [ | ||
| { | ||
| state: ChannelState.UPGRADE_TRY, | ||
| counterparty_chain_id: origin_connection.counterparty_chain_id, | ||
| counterparty_port_id: event.channelUpgradeInfo.dstPortId, | ||
| counterparty_channel_id: event.channelUpgradeInfo.dstChannelId, | ||
| upgrade_sequence: event.channelUpgradeInfo.upgradeSequence, | ||
| }, | ||
| ]) // remove INIT from the dst chain | ||
| insert(DB, ChannelUpgradeController.tableName, channelUpgradeAck) // store ACK state to the src chain | ||
| } |
There was a problem hiding this comment.
Bug: wrong deletion criteria — removes nothing (uses counterparty_ instead of chain/port/channel).*
The INIT→TRY transition tries to delete the previously inserted TRY row, but the where-clause targets counterparty_* fields. The row created in feedUpgradeInitEvent stores TRY with:
- chain_id = origin_connection.counterparty_chain_id
- port_id = dstPortId
- channel_id = dstChannelId
Using counterparty_* won’t match that row, leaving stale entries and causing duplicate/conflicting work. Also, the comment says “remove INIT” but code removes TRY.
Fix the where-clause to match chain/port/channel and correct the comment.
Apply this diff:
- del<ChannelUpgradeTable>(DB, ChannelUpgradeController.tableName, [
- {
- state: ChannelState.UPGRADE_TRY,
- counterparty_chain_id: origin_connection.counterparty_chain_id,
- counterparty_port_id: event.channelUpgradeInfo.dstPortId,
- counterparty_channel_id: event.channelUpgradeInfo.dstChannelId,
- upgrade_sequence: event.channelUpgradeInfo.upgradeSequence,
- },
- ]) // remove INIT from the dst chain
+ del<ChannelUpgradeTable>(DB, ChannelUpgradeController.tableName, [
+ {
+ state: ChannelState.UPGRADE_TRY,
+ chain_id: origin_connection.counterparty_chain_id,
+ port_id: event.channelUpgradeInfo.dstPortId,
+ channel_id: event.channelUpgradeInfo.dstChannelId,
+ upgrade_sequence: event.channelUpgradeInfo.upgradeSequence,
+ },
+ ]) // remove TRY from the dst chain📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return () => { | |
| del<ChannelUpgradeTable>(DB, ChannelUpgradeController.tableName, [ | |
| { | |
| state: ChannelState.UPGRADE_TRY, | |
| counterparty_chain_id: origin_connection.counterparty_chain_id, | |
| counterparty_port_id: event.channelUpgradeInfo.dstPortId, | |
| counterparty_channel_id: event.channelUpgradeInfo.dstChannelId, | |
| upgrade_sequence: event.channelUpgradeInfo.upgradeSequence, | |
| }, | |
| ]) // remove INIT from the dst chain | |
| insert(DB, ChannelUpgradeController.tableName, channelUpgradeAck) // store ACK state to the src chain | |
| } | |
| return () => { | |
| del<ChannelUpgradeTable>(DB, ChannelUpgradeController.tableName, [ | |
| { | |
| state: ChannelState.UPGRADE_TRY, | |
| chain_id: origin_connection.counterparty_chain_id, | |
| port_id: event.channelUpgradeInfo.dstPortId, | |
| channel_id: event.channelUpgradeInfo.dstChannelId, | |
| upgrade_sequence: event.channelUpgradeInfo.upgradeSequence, | |
| }, | |
| ]) // remove TRY from the dst chain | |
| insert(DB, ChannelUpgradeController.tableName, channelUpgradeAck) // store ACK state to the src chain | |
| } |
🤖 Prompt for AI Agents
In src/db/controller/channelUpgrade.ts around lines 157 to 168, the deletion
uses counterparty_* fields and a wrong comment; change the where-clause to
delete rows by chain_id, port_id, channel_id (not counterparty_*) using
origin_connection.counterparty_chain_id, event.channelUpgradeInfo.dstPortId, and
event.channelUpgradeInfo.dstChannelId and ensure the state is
ChannelState.UPGRADE_TRY, and update the trailing comment to "remove TRY from
the dst chain"; leave the subsequent insert of ACK unchanged.
| return () => { | ||
| del<ChannelUpgradeTable>(DB, ChannelUpgradeController.tableName, [ | ||
| { | ||
| state: ChannelState.UPGRADE_ACK, | ||
| counterparty_chain_id: origin_connection.counterparty_chain_id, | ||
| counterparty_port_id: event.channelUpgradeInfo.dstPortId, | ||
| counterparty_channel_id: event.channelUpgradeInfo.dstChannelId, | ||
| upgrade_sequence: event.channelUpgradeInfo.upgradeSequence, | ||
| }, | ||
| ]) // remove ACK from the src chain | ||
| insert(DB, ChannelUpgradeController.tableName, channelUpgradeConfirm) // store CONFIRM state to the dst chain | ||
| } |
There was a problem hiding this comment.
Bug: wrong deletion criteria — ACK row not removed (same root cause).
The TRY→ACK transition must delete the ACK row that was created for the opposite side (chain_id on dst), but the filter again uses counterparty_* fields. This will not match, leaving stale ACK entries.
Apply this diff:
- del<ChannelUpgradeTable>(DB, ChannelUpgradeController.tableName, [
- {
- state: ChannelState.UPGRADE_ACK,
- counterparty_chain_id: origin_connection.counterparty_chain_id,
- counterparty_port_id: event.channelUpgradeInfo.dstPortId,
- counterparty_channel_id: event.channelUpgradeInfo.dstChannelId,
- upgrade_sequence: event.channelUpgradeInfo.upgradeSequence,
- },
- ]) // remove ACK from the src chain
+ del<ChannelUpgradeTable>(DB, ChannelUpgradeController.tableName, [
+ {
+ state: ChannelState.UPGRADE_ACK,
+ chain_id: origin_connection.counterparty_chain_id,
+ port_id: event.channelUpgradeInfo.dstPortId,
+ channel_id: event.channelUpgradeInfo.dstChannelId,
+ upgrade_sequence: event.channelUpgradeInfo.upgradeSequence,
+ },
+ ]) // remove ACK from the src chain📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return () => { | |
| del<ChannelUpgradeTable>(DB, ChannelUpgradeController.tableName, [ | |
| { | |
| state: ChannelState.UPGRADE_ACK, | |
| counterparty_chain_id: origin_connection.counterparty_chain_id, | |
| counterparty_port_id: event.channelUpgradeInfo.dstPortId, | |
| counterparty_channel_id: event.channelUpgradeInfo.dstChannelId, | |
| upgrade_sequence: event.channelUpgradeInfo.upgradeSequence, | |
| }, | |
| ]) // remove ACK from the src chain | |
| insert(DB, ChannelUpgradeController.tableName, channelUpgradeConfirm) // store CONFIRM state to the dst chain | |
| } | |
| return () => { | |
| del<ChannelUpgradeTable>(DB, ChannelUpgradeController.tableName, [ | |
| { | |
| state: ChannelState.UPGRADE_ACK, | |
| chain_id: origin_connection.counterparty_chain_id, | |
| port_id: event.channelUpgradeInfo.dstPortId, | |
| channel_id: event.channelUpgradeInfo.dstChannelId, | |
| upgrade_sequence: event.channelUpgradeInfo.upgradeSequence, | |
| }, | |
| ]) // remove ACK from the src chain | |
| insert(DB, ChannelUpgradeController.tableName, channelUpgradeConfirm) // store CONFIRM state to the dst chain | |
| } |
🤖 Prompt for AI Agents
In src/db/controller/channelUpgrade.ts around lines 207-218, the deletion filter
uses counterparty_* fields so it doesn't match the ACK row stored for the
opposite side; change the filter keys to target the dst-side row by using
chain_id (set to origin_connection.counterparty_chain_id), port_id
(event.channelUpgradeInfo.dstPortId), and channel_id
(event.channelUpgradeInfo.dstChannelId) instead of
counterparty_chain_id/counterparty_port_id/counterparty_channel_id, keeping
upgrade_sequence as-is so the ACK row for the dst chain is correctly removed
before inserting the CONFIRM.
| CREATE TABLE IF NOT EXISTS channel_upgrade ( | ||
| id INTEGER PRIMARY KEY AUTOINCREMENT, | ||
| in_progress BOOLEAN, | ||
| state INTEGER NOT NULL, -- State enum: UPGRADE_TRY=5, UPGRADE_ACK=6, UPGRADE_CONFIRM=7, UPGRADE_OPEN=8, UPGRADE_ERROR=9 | ||
|
|
||
| chain_id TEXT NOT NULL, | ||
| port_id TEXT NOT NULL, | ||
| channel_id TEXT NOT NULL, | ||
| connection_id TEXT NOT NULL, | ||
| upgrade_connection_id TEXT, | ||
|
|
||
| counterparty_chain_id TEXT NOT NULL, | ||
| counterparty_port_id TEXT NOT NULL, | ||
| counterparty_channel_id TEXT NOT NULL, | ||
| counterparty_connection_id TEXT NOT NULL, | ||
| counterparty_upgrade_connection_id TEXT, | ||
|
|
||
| upgrade_sequence BIGINT, | ||
| upgrade_version TEXT, | ||
| upgrade_ordering TEXT, | ||
| upgrade_error_receipt TEXT -- Nullable, only available after ERROR | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Tighten schema: defaults, constraints, and indexes to avoid duplicates and speed up queries.
BOOLEANin SQLite is an alias; store asINTEGERwithCHECK (in_progress IN (0,1))andDEFAULT 0.- Add a
CHECKforstateto prevent invalid values. The spec introducesUPGRADE_TIMEOUT = 10; even if you don’t persist timeouts, document it or include it in the CHECK. - Add a uniqueness guard per upgrade handshake to prevent duplicate rows (race/proc restarts).
- Add indexes for the hot query paths used by workers (filter by
chain_id/state/in_progress, lookups by (port,channel), and counterparty lookups).
Proposed diff (append after the table definition and adjust columns):
CREATE TABLE IF NOT EXISTS channel_upgrade (
id INTEGER PRIMARY KEY AUTOINCREMENT,
- in_progress BOOLEAN,
- state INTEGER NOT NULL, -- State enum: UPGRADE_TRY=5, UPGRADE_ACK=6, UPGRADE_CONFIRM=7, UPGRADE_OPEN=8, UPGRADE_ERROR=9
+ in_progress INTEGER NOT NULL DEFAULT 0, -- 0=false, 1=true
+ state INTEGER NOT NULL, -- State enum: UPGRADE_TRY=5, UPGRADE_ACK=6, UPGRADE_CONFIRM=7, UPGRADE_OPEN=8, UPGRADE_ERROR=9 (UPGRADE_TIMEOUT=10 is not persisted)
chain_id TEXT NOT NULL,
port_id TEXT NOT NULL,
channel_id TEXT NOT NULL,
connection_id TEXT NOT NULL,
upgrade_connection_id TEXT,
counterparty_chain_id TEXT NOT NULL,
counterparty_port_id TEXT NOT NULL,
counterparty_channel_id TEXT NOT NULL,
counterparty_connection_id TEXT NOT NULL,
counterparty_upgrade_connection_id TEXT,
- upgrade_sequence BIGINT,
+ upgrade_sequence INTEGER, -- SQLite INTEGER is 64-bit
upgrade_version TEXT,
upgrade_ordering TEXT,
upgrade_error_receipt TEXT -- Nullable, only available after ERROR
-);
+ ,
+ CHECK (in_progress IN (0,1)),
+ CHECK (state BETWEEN 5 AND 10)
+);
+
+-- Uniqueness: at most one record for a given (chain, port, channel, sequence)
+CREATE UNIQUE INDEX IF NOT EXISTS ux_channel_upgrade_handshake
+ ON channel_upgrade (chain_id, port_id, channel_id, upgrade_sequence);
+
+-- Fast selection for workers
+CREATE INDEX IF NOT EXISTS idx_channel_upgrade_chain_state_progress
+ ON channel_upgrade (chain_id, state, in_progress);
+CREATE INDEX IF NOT EXISTS idx_channel_upgrade_src_ids
+ ON channel_upgrade (chain_id, port_id, channel_id);
+CREATE INDEX IF NOT EXISTS idx_channel_upgrade_counterparty_ids
+ ON channel_upgrade (counterparty_chain_id, counterparty_port_id, counterparty_channel_id);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CREATE TABLE IF NOT EXISTS channel_upgrade ( | |
| id INTEGER PRIMARY KEY AUTOINCREMENT, | |
| in_progress BOOLEAN, | |
| state INTEGER NOT NULL, -- State enum: UPGRADE_TRY=5, UPGRADE_ACK=6, UPGRADE_CONFIRM=7, UPGRADE_OPEN=8, UPGRADE_ERROR=9 | |
| chain_id TEXT NOT NULL, | |
| port_id TEXT NOT NULL, | |
| channel_id TEXT NOT NULL, | |
| connection_id TEXT NOT NULL, | |
| upgrade_connection_id TEXT, | |
| counterparty_chain_id TEXT NOT NULL, | |
| counterparty_port_id TEXT NOT NULL, | |
| counterparty_channel_id TEXT NOT NULL, | |
| counterparty_connection_id TEXT NOT NULL, | |
| counterparty_upgrade_connection_id TEXT, | |
| upgrade_sequence BIGINT, | |
| upgrade_version TEXT, | |
| upgrade_ordering TEXT, | |
| upgrade_error_receipt TEXT -- Nullable, only available after ERROR | |
| ); | |
| -- src/db/migration/0.0.5.sql | |
| CREATE TABLE IF NOT EXISTS channel_upgrade ( | |
| id INTEGER PRIMARY KEY AUTOINCREMENT, | |
| in_progress INTEGER NOT NULL DEFAULT 0, -- 0=false, 1=true | |
| state INTEGER NOT NULL, -- State enum: UPGRADE_TRY=5, UPGRADE_ACK=6, UPGRADE_CONFIRM=7, UPGRADE_OPEN=8, UPGRADE_ERROR=9 (UPGRADE_TIMEOUT=10 is not persisted) | |
| chain_id TEXT NOT NULL, | |
| port_id TEXT NOT NULL, | |
| channel_id TEXT NOT NULL, | |
| connection_id TEXT NOT NULL, | |
| upgrade_connection_id TEXT, | |
| counterparty_chain_id TEXT NOT NULL, | |
| counterparty_port_id TEXT NOT NULL, | |
| counterparty_channel_id TEXT NOT NULL, | |
| counterparty_connection_id TEXT NOT NULL, | |
| counterparty_upgrade_connection_id TEXT, | |
| upgrade_sequence INTEGER, -- SQLite INTEGER is 64-bit | |
| upgrade_version TEXT, | |
| upgrade_ordering TEXT, | |
| upgrade_error_receipt TEXT, -- Nullable, only available after ERROR | |
| CHECK (in_progress IN (0,1)), | |
| CHECK (state BETWEEN 5 AND 10) | |
| ); | |
| -- Uniqueness: at most one record for a given (chain, port, channel, sequence) | |
| CREATE UNIQUE INDEX IF NOT EXISTS ux_channel_upgrade_handshake | |
| ON channel_upgrade (chain_id, port_id, channel_id, upgrade_sequence); | |
| -- Fast selection for workers | |
| CREATE INDEX IF NOT EXISTS idx_channel_upgrade_chain_state_progress | |
| ON channel_upgrade (chain_id, state, in_progress); | |
| CREATE INDEX IF NOT EXISTS idx_channel_upgrade_src_ids | |
| ON channel_upgrade (chain_id, port_id, channel_id); | |
| CREATE INDEX IF NOT EXISTS idx_channel_upgrade_counterparty_ids | |
| ON channel_upgrade (counterparty_chain_id, counterparty_port_id, counterparty_channel_id); |
🤖 Prompt for AI Agents
In src/db/migration/0.0.5.sql around lines 5-26, tighten the channel_upgrade
schema: replace BOOLEAN in_progress with INTEGER DEFAULT 0 and add CHECK
(in_progress IN (0,1)); add a CHECK constraint on state to restrict values to
the enum range (e.g. BETWEEN 5 AND 10 to include UPGRADE_TRY..UPGRADE_TIMEOUT);
add a UNIQUE constraint to guard against duplicate upgrade handshakes (e.g. on
the identifying columns for an upgrade handshake such as chain_id, port_id,
channel_id, connection_id, upgrade_sequence); and create indexes for hot query
paths—(chain_id, state, in_progress), (port_id, channel_id), and
(counterparty_chain_id, counterparty_port_id, counterparty_channel_id)—to speed
worker filtering and lookups.
| export function parseChannelUpgradeEvent( | ||
| event: Event, | ||
| height: number | ||
| ): ChannelUpgradeInfo { | ||
| const portId = find(event, 'port_id') as string | ||
| const channelId = find(event, 'channel_id') as string | ||
| const counterpartyPortId = find(event, 'counterparty_port_id') as string | ||
| const counterpartyChannelId = find(event, 'counterparty_channel_id') as string | ||
| const upgradeSequence = find(event, 'upgrade_sequence') as string | ||
| const upgradeVersion = find(event, 'version') | ||
| const upgradeOrdering = find(event, 'ordering') | ||
| const upgradeConnectionHops = find(event, 'connection_hops') | ||
| const upgradeErrorReceipt = find(event, 'error_receipt') | ||
| const channelState = find(event, 'channel_state') | ||
|
|
||
| return { | ||
| height, | ||
| channelState, | ||
| srcPortId: portId, | ||
| srcChannelId: channelId, | ||
| dstPortId: counterpartyPortId, | ||
| dstChannelId: counterpartyChannelId, | ||
| upgradeSequence: parseInt(upgradeSequence), | ||
| upgradeVersion, | ||
| upgradeOrdering, | ||
| upgradeConnectionHops, | ||
| upgradeErrorReceipt, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix src/dst mapping for upgrade events and make sequence parsing robust.
- For upgrade events, src/dst flips by event type (init/ack/open are “src”; try/confirm are “dst”). Current code always treats
port_id/channel_idas src, which will invert identifiers fortryandconfirm. parseInt('')yieldsNaN. Prefer an optional number with a safe fallback or explicitly handle absence.- Consider reading connection hops via multiple keys for cross-version compatibility.
Proposed refactor:
export function parseChannelUpgradeEvent(
event: Event,
height: number
): ChannelUpgradeInfo {
- const portId = find(event, 'port_id') as string
- const channelId = find(event, 'channel_id') as string
- const counterpartyPortId = find(event, 'counterparty_port_id') as string
- const counterpartyChannelId = find(event, 'counterparty_channel_id') as string
- const upgradeSequence = find(event, 'upgrade_sequence') as string
- const upgradeVersion = find(event, 'version')
- const upgradeOrdering = find(event, 'ordering')
- const upgradeConnectionHops = find(event, 'connection_hops')
- const upgradeErrorReceipt = find(event, 'error_receipt')
- const channelState = find(event, 'channel_state')
+ const portId = find(event, 'port_id') as string
+ const channelId = find(event, 'channel_id') as string
+ const counterpartyPortId = find(event, 'counterparty_port_id') as string
+ const counterpartyChannelId = find(event, 'counterparty_channel_id') as string
+ const upgradeSequenceRaw = find(event, 'upgrade_sequence') // may be ''
+ const upgradeVersion = find(event, 'version')
+ const upgradeOrdering = find(event, 'ordering')
+ // support potential variants of connection_hops across ibc-go versions
+ const upgradeConnectionHops =
+ find(event, 'connection_hops') ??
+ find(event, 'connection_hops[0]') ??
+ find(event, 'ConnectionHops[0]')
+ const upgradeErrorReceipt = find(event, 'error_receipt')
+ const channelState = find(event, 'channel_state')
+
+ // init/ack/open are from src; try/confirm are from dst
+ const isSrc =
+ event.type === 'channel_upgrade_init' ||
+ event.type === 'channel_upgrade_ack' ||
+ event.type === 'channel_upgrade_open'
+
+ const srcPortId = isSrc ? portId : counterpartyPortId
+ const srcChannelId = isSrc ? channelId : counterpartyChannelId
+ const dstPortId = isSrc ? counterpartyPortId : portId
+ const dstChannelId = isSrc ? counterpartyChannelId : channelId
+
+ // Safe upgradeSequence parsing
+ const upgradeSequence =
+ upgradeSequenceRaw && upgradeSequenceRaw !== ''
+ ? Number(upgradeSequenceRaw)
+ : undefined
return {
height,
channelState,
- srcPortId: portId,
- srcChannelId: channelId,
- dstPortId: counterpartyPortId,
- dstChannelId: counterpartyChannelId,
- upgradeSequence: parseInt(upgradeSequence),
+ srcPortId,
+ srcChannelId,
+ dstPortId,
+ dstChannelId,
+ upgradeSequence,
upgradeVersion,
upgradeOrdering,
upgradeConnectionHops,
upgradeErrorReceipt,
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function parseChannelUpgradeEvent( | |
| event: Event, | |
| height: number | |
| ): ChannelUpgradeInfo { | |
| const portId = find(event, 'port_id') as string | |
| const channelId = find(event, 'channel_id') as string | |
| const counterpartyPortId = find(event, 'counterparty_port_id') as string | |
| const counterpartyChannelId = find(event, 'counterparty_channel_id') as string | |
| const upgradeSequence = find(event, 'upgrade_sequence') as string | |
| const upgradeVersion = find(event, 'version') | |
| const upgradeOrdering = find(event, 'ordering') | |
| const upgradeConnectionHops = find(event, 'connection_hops') | |
| const upgradeErrorReceipt = find(event, 'error_receipt') | |
| const channelState = find(event, 'channel_state') | |
| return { | |
| height, | |
| channelState, | |
| srcPortId: portId, | |
| srcChannelId: channelId, | |
| dstPortId: counterpartyPortId, | |
| dstChannelId: counterpartyChannelId, | |
| upgradeSequence: parseInt(upgradeSequence), | |
| upgradeVersion, | |
| upgradeOrdering, | |
| upgradeConnectionHops, | |
| upgradeErrorReceipt, | |
| } | |
| } | |
| export function parseChannelUpgradeEvent( | |
| event: Event, | |
| height: number | |
| ): ChannelUpgradeInfo { | |
| const portId = find(event, 'port_id') as string | |
| const channelId = find(event, 'channel_id') as string | |
| const counterpartyPortId = find(event, 'counterparty_port_id') as string | |
| const counterpartyChannelId = find(event, 'counterparty_channel_id') as string | |
| const upgradeSequenceRaw = find(event, 'upgrade_sequence') // may be '' | |
| const upgradeVersion = find(event, 'version') | |
| const upgradeOrdering = find(event, 'ordering') | |
| // support potential variants of connection_hops across ibc-go versions | |
| const upgradeConnectionHops = | |
| find(event, 'connection_hops') ?? | |
| find(event, 'connection_hops[0]') ?? | |
| find(event, 'ConnectionHops[0]') | |
| const upgradeErrorReceipt = find(event, 'error_receipt') | |
| const channelState = find(event, 'channel_state') | |
| // init/ack/open are from src; try/confirm are from dst | |
| const isSrc = | |
| event.type === 'channel_upgrade_init' || | |
| event.type === 'channel_upgrade_ack' || | |
| event.type === 'channel_upgrade_open' | |
| const srcPortId = isSrc ? portId : counterpartyPortId | |
| const srcChannelId = isSrc ? channelId : counterpartyChannelId | |
| const dstPortId = isSrc ? counterpartyPortId : portId | |
| const dstChannelId = isSrc ? counterpartyChannelId : channelId | |
| // Safe upgradeSequence parsing | |
| const upgradeSequence = | |
| upgradeSequenceRaw && upgradeSequenceRaw !== '' | |
| ? Number(upgradeSequenceRaw) | |
| : undefined | |
| return { | |
| height, | |
| channelState, | |
| srcPortId, | |
| srcChannelId, | |
| dstPortId, | |
| dstChannelId, | |
| upgradeSequence, | |
| upgradeVersion, | |
| upgradeOrdering, | |
| upgradeConnectionHops, | |
| upgradeErrorReceipt, | |
| } | |
| } |
| async getUpgrade( | ||
| portId: string, | ||
| channelId: string | ||
| ): Promise<Upgrade | undefined> { | ||
| try { | ||
| const upgrade_response = await this.c.get<UpgradeResponse>( | ||
| `/ibc/core/channel/v1/channels/${channelId}/ports/${portId}/upgrade` | ||
| ) | ||
| return Upgrade.fromData(upgrade_response.upgrade) | ||
| } catch (e) { | ||
| if (e.status === 404) { | ||
| return undefined | ||
| } | ||
|
|
||
| throw e | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
404 handling likely incorrect: e.status is seldom set; check e.response?.status
APIRequester (axios-like) usually surfaces HTTP status under error.response.status. As written, 404s will bubble instead of returning undefined.
Harden error checks and retain type guard:
- } catch (e) {
- if (e.status === 404) {
- return undefined
- }
-
- throw e
- }
+ } catch (e: any) {
+ const status = e?.response?.status ?? e?.status
+ if (status === 404) return undefined
+ throw e
+ }Apply the same change to getUpgradeError.
Also applies to: 103-119
🏁 Script executed:
#!/bin/bash
# Ensure no other places rely on e.status directly
rg -nP --type ts '\.status\s*===\s*404|catch\s*\(\s*e' src/lib/restClient.ts -n -C2Length of output: 437
Harden 404 error handling in getUpgrade and getUpgradeError
Both methods currently check e.status === 404, but Axios-style errors surface the HTTP code on e.response.status, so 404s will still bubble up. Update both catch blocks to coalesce on response?.status first, then fallback to e.status.
• File src/lib/restClient.ts, around line 94 (in getUpgrade)
• File src/lib/restClient.ts, around line 112 (in getUpgradeError)
Proposed diff for each block:
- } catch (e) {
- if (e.status === 404) {
- return undefined
- }
-
- throw e
- }
+ } catch (e: any) {
+ const status = e?.response?.status ?? e?.status
+ if (status === 404) return undefined
+ throw e
+ }Apply this change in both locations to ensure 404 responses return undefined as intended.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async getUpgrade( | |
| portId: string, | |
| channelId: string | |
| ): Promise<Upgrade | undefined> { | |
| try { | |
| const upgrade_response = await this.c.get<UpgradeResponse>( | |
| `/ibc/core/channel/v1/channels/${channelId}/ports/${portId}/upgrade` | |
| ) | |
| return Upgrade.fromData(upgrade_response.upgrade) | |
| } catch (e) { | |
| if (e.status === 404) { | |
| return undefined | |
| } | |
| throw e | |
| } | |
| } | |
| async getUpgrade( | |
| portId: string, | |
| channelId: string | |
| ): Promise<Upgrade | undefined> { | |
| try { | |
| const upgrade_response = await this.c.get<UpgradeResponse>( | |
| `/ibc/core/channel/v1/channels/${channelId}/ports/${portId}/upgrade` | |
| ) | |
| return Upgrade.fromData(upgrade_response.upgrade) | |
| } catch (e: any) { | |
| const status = e?.response?.status ?? e?.status | |
| if (status === 404) return undefined | |
| throw e | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/lib/restClient.ts around lines 85-101 and again around line 112, the
catch blocks only check e.status === 404 which misses Axios-style errors that
store the HTTP code at e.response.status; change each catch to extract the
status with e?.response?.status ?? e?.status (cast e to any if needed), then if
that status === 404 return undefined, otherwise rethrow the error. Ensure the
variable name is used consistently and keep the original behavior for non-404
errors.
| private async filterChannelUpgradeEvents( | ||
| channelUpgrades: ChannelUpgradeTable[] | ||
| ): Promise<ChannelUpgradeTable[]> { | ||
| const eventsToDel: ChannelUpgradeTable[] = [] | ||
|
|
||
| // check already executed | ||
| const res = await Promise.all( | ||
| channelUpgrades.map(async (v) => { | ||
| const counterpartyChain = | ||
| this.workerController.chains[v.counterparty_chain_id] | ||
| const channel = | ||
| v.channel_id !== '' | ||
| ? await this.chain.rest.ibc.channel(v.port_id, v.channel_id) | ||
| : undefined | ||
| const counterpartyChannel = | ||
| v.counterparty_channel_id !== '' | ||
| ? await counterpartyChain.rest.ibc.channel( | ||
| v.counterparty_port_id, | ||
| v.counterparty_channel_id | ||
| ) | ||
| : undefined | ||
|
|
||
| const upgradeSequence = parseInt( | ||
| channel?.channel.upgrade_sequence || '0' | ||
| ) | ||
| const counterpartyUpgradeSequence = parseInt( | ||
| counterpartyChannel?.channel.upgrade_sequence || '0' | ||
| ) | ||
|
|
||
| const upgrade = await this.chain.rest.ibc.getUpgrade( | ||
| v.port_id, | ||
| v.channel_id | ||
| ) | ||
| const counterpartyUpgrade = await counterpartyChain.rest.ibc.getUpgrade( | ||
| v.counterparty_port_id, | ||
| v.counterparty_channel_id | ||
| ) | ||
|
|
||
| const channelState = stateFromJSON(channel?.channel.state) | ||
| const counterpartyChannelState = stateFromJSON( | ||
| counterpartyChannel?.channel.state | ||
| ) | ||
|
|
||
| // Check both our internal upgrade state AND the actual IBC channel state | ||
| switch (v.state) { | ||
| case ChannelState.UPGRADE_TRY: | ||
| if ( | ||
| channel && | ||
| counterpartyChannel && | ||
| counterpartyUpgradeSequence === upgradeSequence + 1 && | ||
| counterpartyUpgradeSequence === (v.upgrade_sequence as number) && | ||
| channelState === State.STATE_OPEN | ||
| ) { | ||
| return v | ||
| } | ||
| break | ||
| case ChannelState.UPGRADE_ACK: | ||
| // if upgrade sequence is different, then ignore | ||
| // if channel state is not open, then ignore | ||
| // if counterparty upgrade sequence is different, then ignore | ||
| if ( | ||
| channel && | ||
| counterpartyChannel && | ||
| upgrade && | ||
| counterpartyUpgrade && | ||
| upgradeSequence === counterpartyUpgradeSequence && | ||
| upgradeSequence === (v.upgrade_sequence as number) && | ||
| (channelState === State.STATE_OPEN || | ||
| channelState === State.STATE_FLUSHING) | ||
| ) { | ||
| return v | ||
| } | ||
| break | ||
| case ChannelState.UPGRADE_CONFIRM: | ||
| // if upgrade sequence is different, then ignore | ||
| // if channel state is not flushing or flush complete, then ignore | ||
| // if counterparty upgrade sequence is different, then ignore | ||
| if ( | ||
| channel && | ||
| counterpartyChannel && | ||
| upgrade && | ||
| counterpartyUpgrade && | ||
| upgradeSequence === counterpartyUpgradeSequence && | ||
| upgradeSequence === (v.upgrade_sequence as number) && | ||
| channelState === State.STATE_FLUSHING && | ||
| (counterpartyChannelState === State.STATE_FLUSHING || | ||
| counterpartyChannelState === State.STATE_FLUSHCOMPLETE) | ||
| ) { | ||
| return v | ||
| } | ||
| break | ||
| case ChannelState.UPGRADE_OPEN: | ||
| // if upgrade sequence is different, then ignore | ||
| // if counterparty upgrade sequence is different, then ignore | ||
| // if both chain state is flushing, then wait for flush complete | ||
| // if channel state is not flush complete, then ignore | ||
| // if counterparty channel state is not flush complete or open, then ignore | ||
| if ( | ||
| channel && | ||
| counterpartyChannel && | ||
| upgrade && | ||
| upgradeSequence === counterpartyUpgradeSequence && | ||
| upgradeSequence === (v.upgrade_sequence as number) && | ||
| channelState === State.STATE_FLUSHCOMPLETE && | ||
| (counterpartyChannelState === State.STATE_FLUSHCOMPLETE || | ||
| counterpartyChannelState === State.STATE_OPEN) | ||
| ) { | ||
| // During the UPGRADE_OPEN step, the counterparty channel has already switched to using | ||
| // the new connection. However, we need to use the original connection for relaying this | ||
| // event since the upgrade is not yet complete on the current chain side. | ||
| // Get the original connection from the channel's connection_hops and update the event's | ||
| // connection IDs accordingly. | ||
| const oldConnection = await ConnectionController.getConnection( | ||
| this.chain.rest, | ||
| this.chain.chainId, | ||
| channel.channel.connection_hops[0] | ||
| ) | ||
| v.connection_id = oldConnection.connection_id | ||
| v.counterparty_connection_id = | ||
| oldConnection.counterparty_connection_id | ||
|
|
||
| return v | ||
| } else if ( | ||
| channel && | ||
| counterpartyChannel && | ||
| upgrade && | ||
| counterpartyUpgrade && | ||
| upgradeSequence === counterpartyUpgradeSequence && | ||
| upgradeSequence === (v.upgrade_sequence as number) && | ||
| (channelState === State.STATE_FLUSHING || | ||
| counterpartyChannelState === State.STATE_FLUSHING) | ||
| ) { | ||
| // need to wait for flush complete | ||
| return undefined | ||
| } | ||
| break | ||
| case ChannelState.UPGRADE_TIMEOUT: | ||
| // if upgrade sequence is different, then ignore | ||
| // if channel state is not flushing or flush complete, then ignore | ||
| if ( | ||
| channel && | ||
| upgrade && | ||
| upgradeSequence === (v.upgrade_sequence as number) && | ||
| (channelState === State.STATE_FLUSHING || | ||
| channelState === State.STATE_FLUSHCOMPLETE) | ||
| ) { | ||
| return v | ||
| } | ||
| break | ||
| case ChannelState.UPGRADE_ERROR: | ||
| // if there is no error receipt in the chain state, return v | ||
| const errorReceipt = await this.chain.rest.ibc.getUpgradeError( | ||
| v.port_id, | ||
| v.channel_id | ||
| ) | ||
| const counterpartyErrorReceipt = | ||
| await counterpartyChain.rest.ibc.getUpgradeError( | ||
| v.counterparty_port_id, | ||
| v.counterparty_channel_id | ||
| ) | ||
|
|
||
| // if upgrade sequence is different, then ignore | ||
| // if there is no error receipt in the counterparty chain state, then ignore | ||
| // if there is error receipt and sequence is same or higher, then ignore | ||
| if ( | ||
| upgrade && | ||
| (!errorReceipt || | ||
| errorReceipt.sequence < (v.upgrade_sequence || 0)) && | ||
| counterpartyErrorReceipt && | ||
| counterpartyErrorReceipt.sequence === | ||
| (v.upgrade_sequence as number) && | ||
| upgradeSequence === (v.upgrade_sequence as number) | ||
| ) { | ||
| return v | ||
| } | ||
| break | ||
| } | ||
|
|
||
| eventsToDel.push(v) | ||
|
|
||
| return undefined | ||
| }) | ||
| ) | ||
|
|
||
| // Clean up completed/failed upgrades | ||
| for (const upgrade of eventsToDel) { | ||
| if (upgrade.id) { | ||
| ChannelUpgradeController.deleteUpgrade(upgrade.id) | ||
| } | ||
| } | ||
|
|
||
| return res.filter((v) => v !== undefined) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Complex upgrade filtering logic needs comprehensive testing
The filterChannelUpgradeEvents method contains complex state machine logic with many edge cases. Ensure comprehensive unit tests cover all state transitions and edge cases, particularly:
- State transition validation
- Sequence number matching
- Timeout scenarios
- Error receipt handling
- Connection ID remapping
🏁 Script executed:
#!/bin/bash
# Check for existing tests for filterChannelUpgradeEvents
fd -e test.ts -e spec.ts . --exec grep -l "filterChannelUpgradeEvents" {} \;Length of output: 79
🏁 Script executed:
#!/bin/bash
# Broad search for filterChannelUpgradeEvents in test files
rg -n "filterChannelUpgradeEvents" -g "*.test.ts" -g "*.spec.ts" -g "*.test.js" -g "*.spec.js"Length of output: 94
Add comprehensive unit tests for filterChannelUpgradeEvents
A search of existing test files did not find any references to filterChannelUpgradeEvents, indicating this critical logic isn’t covered by unit tests. Please add tests that exercise each branch and edge case in this method, located in src/workers/wallet.ts (lines 949–1141):
• State transition validation for every ChannelState case (UPGRADE_TRY, UPGRADE_ACK, UPGRADE_CONFIRM, UPGRADE_OPEN, UPGRADE_TIMEOUT, UPGRADE_ERROR)
• Sequence number matching and mismatches
• Timeout scenarios (flushing vs. flush complete)
• Error receipt handling when upgrade errors occur or are absent
• Connection ID remapping logic in the UPGRADE_OPEN branch
Aim for a new test file (e.g. src/workers/__tests__/wallet.spec.ts) that mocks the rest.ibc calls and covers both “pass through” and “filter out” outcomes for each scenario.
🧰 Tools
🪛 Biome (2.1.2)
[error] 1100-1103: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1104-1108: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
In src/workers/wallet.ts around lines 949–1141, add comprehensive Jest unit
tests for filterChannelUpgradeEvents in a new file
src/workers/__tests__/wallet.spec.ts that mock this.chain.rest.ibc, counterparty
chain rest.ibc, ConnectionController.getConnection and
ChannelUpgradeController.deleteUpgrade and provide a fake
workerController.chains; create test cases exercising every ChannelState branch
(UPGRADE_TRY, UPGRADE_ACK, UPGRADE_CONFIRM, UPGRADE_OPEN, UPGRADE_TIMEOUT,
UPGRADE_ERROR) with both matching and mismatched upgrade_sequence values,
flushing vs flush complete scenarios, presence/absence of error receipts, and
the UPGRADE_OPEN connection ID remapping path, asserting returned array includes
only valid events and that deleteUpgrade is called for filtered events and that
connection IDs are updated when expected.
| const errorReceipt = await this.chain.rest.ibc.getUpgradeError( | ||
| v.port_id, | ||
| v.channel_id | ||
| ) | ||
| const counterpartyErrorReceipt = | ||
| await counterpartyChain.rest.ibc.getUpgradeError( | ||
| v.counterparty_port_id, | ||
| v.counterparty_channel_id | ||
| ) |
There was a problem hiding this comment.
Fix variable declarations in switch cases
The static analysis correctly identified that variable declarations in switch cases can be accessed by other cases. Wrap them in blocks for proper scoping.
case ChannelState.UPGRADE_ERROR:
+ {
// if there is no error receipt in the chain state, return v
const errorReceipt = await this.chain.rest.ibc.getUpgradeError(
v.port_id,
v.channel_id
)
const counterpartyErrorReceipt =
await counterpartyChain.rest.ibc.getUpgradeError(
v.counterparty_port_id,
v.counterparty_channel_id
)
// if upgrade sequence is different, then ignore
// if there is no error receipt in the counterparty chain state, then ignore
// if there is error receipt and sequence is same or higher, then ignore
if (
upgrade &&
(!errorReceipt ||
errorReceipt.sequence < (v.upgrade_sequence || 0)) &&
counterpartyErrorReceipt &&
counterpartyErrorReceipt.sequence ===
(v.upgrade_sequence as number) &&
upgradeSequence === (v.upgrade_sequence as number)
) {
return v
}
break
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const errorReceipt = await this.chain.rest.ibc.getUpgradeError( | |
| v.port_id, | |
| v.channel_id | |
| ) | |
| const counterpartyErrorReceipt = | |
| await counterpartyChain.rest.ibc.getUpgradeError( | |
| v.counterparty_port_id, | |
| v.counterparty_channel_id | |
| ) | |
| case ChannelState.UPGRADE_ERROR: { | |
| // if there is no error receipt in the chain state, return v | |
| const errorReceipt = await this.chain.rest.ibc.getUpgradeError( | |
| v.port_id, | |
| v.channel_id | |
| ) | |
| const counterpartyErrorReceipt = | |
| await counterpartyChain.rest.ibc.getUpgradeError( | |
| v.counterparty_port_id, | |
| v.counterparty_channel_id | |
| ) | |
| // if upgrade sequence is different, then ignore | |
| // if there is no error receipt in the counterparty chain state, then ignore | |
| // if there is error receipt and sequence is same or higher, then ignore | |
| if ( | |
| upgrade && | |
| (!errorReceipt || | |
| errorReceipt.sequence < (v.upgrade_sequence || 0)) && | |
| counterpartyErrorReceipt && | |
| counterpartyErrorReceipt.sequence === | |
| (v.upgrade_sequence as number) && | |
| upgradeSequence === (v.upgrade_sequence as number) | |
| ) { | |
| return v | |
| } | |
| break | |
| } |
🧰 Tools
🪛 Biome (2.1.2)
[error] 1100-1103: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1104-1108: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
In src/workers/wallet.ts around lines 1100 to 1108, the code declares variables
directly inside switch cases (const errorReceipt and const
counterpartyErrorReceipt) which allows cross-case access; wrap the declarations
and their usage in a dedicated block ({ ... }) inside the case to limit scope to
that case, ensuring each const is declared inside its own braces and preserving
existing logic and breaks.
…ut set on src chain
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
src/workers/wallet.ts(18 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
src/workers/wallet.ts
[error] 1097-1100: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1101-1105: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (1)
src/workers/wallet.ts (1)
1096-1121: Wrap switch-case declarations in a blockLine 1101 declares
constbindings directly inside the switch clause, which Biome flags aslint/correctness/noSwitchDeclarations. This remains unresolved from the earlier review and will fail CI. Please wrap the clause body in its own block.- case ChannelState.UPGRADE_ERROR: - // if there is no error receipt in the chain state, return v - const errorReceipt = await this.chain.rest.ibc.getUpgradeError( + case ChannelState.UPGRADE_ERROR: { + // if there is no error receipt in the chain state, return v + const errorReceipt = await this.chain.rest.ibc.getUpgradeError( v.port_id, v.channel_id ) const counterpartyErrorReceipt = await counterpartyChain.rest.ibc.getUpgradeError( v.counterparty_port_id, v.counterparty_channel_id ) @@ if ( upgrade && (!errorReceipt || errorReceipt.sequence < (v.upgrade_sequence || 0)) && counterpartyErrorReceipt && counterpartyErrorReceipt.sequence === (v.upgrade_sequence as number) && upgradeSequence === (v.upgrade_sequence as number) ) { return v } break + }
| const timeout_timestamp = BigInt( | ||
| counterpartyUpgrade.timeout?.timestamp as string | ||
| ) | ||
| if ( | ||
| timeout_timestamp !== undefined && | ||
| timeout_timestamp !== BigInt(0) && | ||
| BigInt(chain.latestTimestamp) * BigInt(1000000) > timeout_timestamp | ||
| ) { | ||
| return true | ||
| } |
There was a problem hiding this comment.
Guard against undefined timeout before calling BigInt
Line 1171 calls BigInt(counterpartyUpgrade.timeout?.timestamp). When the counterparty upgrade has no timeout (common case), that value is undefined, and BigInt(undefined) throws a TypeError, aborting the worker loop. Add an explicit check before conversion.
- const timeout_timestamp = BigInt(
- counterpartyUpgrade.timeout?.timestamp as string
- )
- if (
- timeout_timestamp !== undefined &&
- timeout_timestamp !== BigInt(0) &&
- BigInt(chain.latestTimestamp) * BigInt(1000000) > timeout_timestamp
- ) {
+ const timeoutTimestampStr = counterpartyUpgrade.timeout?.timestamp
+ if (!timeoutTimestampStr) {
+ return false
+ }
+
+ const timeout_timestamp = BigInt(timeoutTimestampStr)
+ if (
+ timeout_timestamp !== BigInt(0) &&
+ BigInt(chain.latestTimestamp) * BigInt(1000000) > timeout_timestamp
+ ) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const timeout_timestamp = BigInt( | |
| counterpartyUpgrade.timeout?.timestamp as string | |
| ) | |
| if ( | |
| timeout_timestamp !== undefined && | |
| timeout_timestamp !== BigInt(0) && | |
| BigInt(chain.latestTimestamp) * BigInt(1000000) > timeout_timestamp | |
| ) { | |
| return true | |
| } | |
| const timeoutTimestampStr = counterpartyUpgrade.timeout?.timestamp | |
| if (!timeoutTimestampStr) { | |
| return false | |
| } | |
| const timeout_timestamp = BigInt(timeoutTimestampStr) | |
| if ( | |
| timeout_timestamp !== BigInt(0) && | |
| BigInt(chain.latestTimestamp) * BigInt(1000000) > timeout_timestamp | |
| ) { | |
| return true | |
| } |
🤖 Prompt for AI Agents
In src/workers/wallet.ts around lines 1170 to 1179, the code calls
BigInt(counterpartyUpgrade.timeout?.timestamp) which throws when timeout or
timestamp is undefined; fix by first checking that counterpartyUpgrade.timeout
and counterpartyUpgrade.timeout.timestamp are defined (e.g., !== undefined)
before converting to BigInt, only perform the BigInt conversion and subsequent
comparisons when the timestamp exists, and otherwise treat the timeout as absent
(skip/return false) so the worker loop does not error.
1 participant
Summary by CodeRabbit
New Features
Documentation
Chores