feat: add client expiration check to update client before expired #20
Conversation
WalkthroughThis pull request introduces enhancements to the client update mechanism across multiple components of the system. The changes focus on improving event parsing, client update tracking, and synchronization processes. A new Changes
Sequence DiagramsequenceDiagram
participant SyncWorker
participant EventParser
participant ClientController
participant WalletWorker
SyncWorker->>EventParser: Parse UpdateClientEvent
EventParser-->>SyncWorker: Return parsed event
SyncWorker->>ClientController: Feed update client event
ClientController-->>SyncWorker: Update client information
WalletWorker->>ClientController: Get clients to update
ClientController-->>WalletWorker: Return clients needing updates
WalletWorker->>WalletWorker: Generate update client messages
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
src/workers/wallet.ts (1)
Line range hint
177-188: Add error handling when fetching connection infoWhen retrieving connection information, if
getConnectionfails, it may cause an unhandled exception. Consider adding error handling to manage potential errors during the asynchronous operations.Apply this diff to handle possible errors:
await Promise.all( connections.map(async (connection) => { try { const connectionInfo = await ConnectionController.getConnection( this.chain.rest, this.chain.chainId, connection ) connectionClientMap[connection] = connectionInfo.client_id + } catch (error) { + this.logger.error(`Failed to get connection info for ${connection}: ${error}`) + // Optionally, handle the error or skip this connection + } }) )src/lib/eventParser.spec.ts (1)
Line range hint
1-1: Add test coverage for parseUpdateClientEvent function.The new parseUpdateClientEvent function lacks test coverage. Consider adding test cases to verify:
- Successful parsing of update_client events
- Handling of missing fields
- Handling of malformed data
Example test structure:
test('update client parser', () => { // test successful parsing { const event = { type: 'update_client', attributes: [ { key: 'client_id', value: 'client-1' }, { key: 'header', value: 'header-data' }, { key: 'consensus_heights', value: '1-1000' } ] } const result = parseUpdateClientEvent(event) expect(result).toEqual({ clientId: 'client-1', header: 'header-data', consensusHeights: '1-1000' }) } // test missing fields { const event = { type: 'update_client', attributes: [ { key: 'client_id', value: 'client-1' } ] } expect(() => parseUpdateClientEvent(event)).toThrow() } })
🧹 Nitpick comments (3)
src/db/controller/client.spec.ts (2)
8-8: Typo in commentThere's a duplicate word "set" in the comment. Please correct it for clarity.
Apply this diff:
mockServers // to set set config file -mockServers // to set set config file +mockServers // to set config file
41-43: Use 'forEach' instead of 'map' for iteration with side effectsSince the result of
mapis not utilized, usingforEachis more appropriate for performing operations with side effects.Apply this diff:
-testClients.map((client) => { +testClients.forEach((client) => { insert(DB, ClientController.tableName, client) })src/lib/eventParser.ts (1)
132-142: LGTM: Well-structured event parsing implementation.The parseUpdateClientEvent function follows the established pattern of other event parsers in the file. It correctly extracts the required fields using the existing find helper function.
However, consider adding error handling for missing required fields.
export function parseUpdateClientEvent(event: Event): UpdateClientEvent { const clientId = find(event, 'client_id') as string const header = find(event, 'header') as string const consensusHeights = find(event, 'consensus_heights') as string + + if (!clientId || !header || !consensusHeights) { + throw new Error('Missing required fields in update client event') + } return { clientId, header, consensusHeights, } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/db/controller/client.spec.ts(1 hunks)src/db/controller/client.ts(3 hunks)src/lib/eventParser.spec.ts(1 hunks)src/lib/eventParser.ts(2 hunks)src/msgs/updateClient.ts(3 hunks)src/workers/chain.ts(6 hunks)src/workers/wallet.ts(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/db/controller/client.ts
[error] 71-71: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 107-107: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🔇 Additional comments (5)
src/db/controller/client.ts (1)
54-54:
Add validation for parsing 'consensusHeights'
The parsing of event.consensusHeights assumes a specific format. If the format is unexpected, it may lead to runtime errors. Consider adding validation to handle potential parsing issues.
Apply this diff to add error handling:
const consensusHeightsArray = event.consensusHeights.split(',')
const revisionHeightParts = consensusHeightsArray[0].split('-')
+if (revisionHeightParts.length < 2) {
+ throw new Error('Invalid consensusHeights format')
+}
const revisionHeight = parseInt(revisionHeightParts[1])Likely invalid or redundant comment.
src/workers/wallet.ts (1)
306-307: Ensure 'msgs' array is populated before proceeding
The check if (msgs.length === 0) return is correctly placed after message generation to avoid unnecessary processing. Good job on optimizing the control flow.
src/lib/eventParser.ts (1)
2-7: LGTM: Clean import statement update.
The import statement is properly updated to include the new UpdateClientEvent type.
src/workers/chain.ts (2)
3-8: LGTM: Clean import updates.
The imports are properly organized to include the new UpdateClientEvent type.
269-272: LGTM: Clean event type handling.
The update_client event type is properly handled and parsed using the new parseUpdateClientEvent function.
…for static variables
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
src/msgs/updateClient.ts (1)
Line range hint
79-89: Improve error handling in the header retrieval loop.The current implementation has a few issues:
- The error is only thrown after 5 retries, but the error message doesn't indicate the retry attempts.
- The delay between retries is fixed at 100ms without exponential backoff.
Consider this improved implementation:
- let header = await chain.rpc.header(height).catch(() => undefined) - let count = 0 - while (header === undefined) { - header = await chain.rpc.header(height).catch((e) => { - if (count > 5) { - throw e - } - return undefined - }) - await delay(100) - count++ - } + const MAX_RETRIES = 5; + const BASE_DELAY = 100; + let retries = 0; + + while (retries <= MAX_RETRIES) { + try { + const header = await chain.rpc.header(height); + if (header) return header; + } catch (e) { + if (retries === MAX_RETRIES) { + throw new Error(`Failed to fetch header after ${MAX_RETRIES} retries: ${e.message}`); + } + await delay(BASE_DELAY * Math.pow(2, retries)); + retries++; + } + }src/db/controller/packet.ts (1)
Line range hint
506-513: Critical: Incorrect table names used in delete operations.Several delete operations are using the wrong table names, which will result in deleting records from the wrong tables:
- In
feedAcknowledgePacketEvent:
- Using
tableNamePacketSendinstead oftableNamePacketTimeout- Using
tableNamePacketSendinstead oftableNamePacketWriteAck- In
feedTimeoutPacketEvent:
- Using
tableNamePacketSendinstead oftableNamePacketTimeoutApply these fixes:
- del<PacketTimeoutTable>(DB, PacketController.tableNamePacketSend, [ + del<PacketTimeoutTable>(DB, PacketController.tableNamePacketTimeout, [ - del<PacketWriteAckTable>(DB, PacketController.tableNamePacketSend, [ + del<PacketWriteAckTable>(DB, PacketController.tableNamePacketWriteAck, [ - del<PacketTimeoutTable>(DB, PacketController.tableNamePacketSend, [ + del<PacketTimeoutTable>(DB, PacketController.tableNamePacketTimeout, [Also applies to: 516-523, 570-577
🧹 Nitpick comments (4)
src/db/controller/client.ts (2)
2-2: Organize Imports for ReadabilityConsider ordering your imports to enhance readability. Typically, external libraries are imported first, followed by internal modules.
69-82: Avoid Magic Strings for Table NamesUsing hard-coded table names can lead to maintenance issues. Consider defining table names as constants or using configuration files.
src/db/controller/packet.ts (2)
318-325: Consider using a transaction for atomic updates.The
resetPacketInProgressmethod updates multiple tables independently. If one update fails, it could leave the system in an inconsistent state.Consider wrapping the updates in a transaction:
public static resetPacketInProgress(db?: Database) { db = db ?? DB + db.transaction(() => { update<PacketSendTable>(db, PacketController.tableNamePacketSend, { in_progress: Bool.FALSE, }) update<PacketTimeoutTable>(db, PacketController.tableNamePacketTimeout, { in_progress: Bool.FALSE, }) update<PacketWriteAckTable>(db, PacketController.tableNamePacketWriteAck, { in_progress: Bool.FALSE, }) + })() }
385-386: Consider using a transaction for atomic inserts.The
feedSendPacketEventmethod inserts records into multiple tables independently. If one insert fails, it could leave the system in an inconsistent state.Consider wrapping the inserts in a transaction:
return () => { + DB.transaction(() => { insert(DB, PacketController.tableNamePacketSend, packetSend) insert(DB, PacketController.tableNamePacketTimeout, packetTimeout) + })()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/db/controller/channel.ts(8 hunks)src/db/controller/client.ts(5 hunks)src/db/controller/connection.ts(2 hunks)src/db/controller/packet.ts(18 hunks)src/db/controller/packetFee.ts(2 hunks)src/db/controller/syncInfo.ts(4 hunks)src/msgs/updateClient.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/db/controller/channel.ts
🔇 Additional comments (10)
src/db/controller/client.ts (1)
91-91: Replace 'this' with Class Name in Static Method
In static methods, using this can be unclear since it refers to the class itself. Replace this with ClientController for clarity.
Apply this diff:
- const client = selectOne<ClientTable>(DB, this.tableName, [
+ const client = selectOne<ClientTable>(DB, ClientController.tableName, [src/db/controller/connection.ts (2)
28-28: Replace 'this' with Class Name in Static Method
Using this in static methods can be confusing. Replace this.tableName with ConnectionController.tableName for clarity.
40-49: Consistent Reference to Class Name
Ensure consistency by replacing this.tableName with ConnectionController.tableName in the getConnection method.
Apply this diff:
- const connection = selectOne<ConnectionTable>(
- DB,
- this.tableName,
+ const connection = selectOne<ConnectionTable>(
+ DB,
+ ConnectionController.tableName,
[
{
chain_id: chainId,
connection_id: connectionId,
},
]
)src/db/controller/packetFee.ts (2)
29-29: Replace 'this' with Class Name in Static Context
In static contexts, use the class name instead of this for clarity. Replace this.tableName with PacketFeeController.tableName.
55-55: Consistent Use of Class Name
Maintain consistency by using PacketFeeController.tableName instead of this.tableName.
src/db/controller/syncInfo.ts (4)
28-28: Replace 'this' with Class Name in Static Method
Replace this.tableName with SyncInfoController.tableName to clarify that the reference is to the class itself.
44-44: Consistent Reference to Class Name
For consistency, update this.tableName to SyncInfoController.tableName when inserting newSyncInfo.
56-58: Use Class Name Instead of 'this' in Static Method
In the getSyncInfos method, replace this.tableName with SyncInfoController.tableName for clarity.
78-92: Verify Logic in Sync Info Update
The update method may not correctly handle merging sync information when syncedHeight equals endHeight.
Consider testing scenarios where syncedHeight reaches endHeight to ensure sync information merges as expected.
src/msgs/updateClient.ts (1)
118-120: LGTM: Address comparison is now case-insensitive.
The updated implementation correctly converts both addresses to lowercase before comparison, preventing case-sensitivity issues.
Features
feedUpdateClientEventfunction.Improvements
rpc.headerinstead ofrest.block_infoto reduce query costs and avoid thegrpc: received message larger than maxerror.Summary by CodeRabbit
New Features
UpdateClientEventin event processing.Bug Fixes
getValidatorSetfunction.Tests
ClientControllerfunctionality.parsePacketEventfunction to align with new specifications.Chores