Feat/protobuf load definitions #17295
Conversation
WalkthroughThe pull request introduces changes that enhance message definition management across multiple packages. It adds a new asynchronous function, ✨ Finishing Touches
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: 0
🧹 Nitpick comments (6)
packages/protobuf/src/load-definitions.ts (6)
3-3: Consider using a more specific type for Definitions.The
Definitionstype is quite generic. A more specific type that better represents the structure of protocol buffer definitions would improve type safety and readability.-type Definitions = Record<string, unknown>; +type Definitions = Record<string, { + values?: Record<string, number>; + [key: string]: unknown; +}>;
10-18: Add comments to explain silent error handling.The function silently catches and ignores errors when checking if a package exists. While this may be intentional, adding a comment explaining why errors are ignored would improve maintainability.
// check if package already exists try { const pkg = messages.lookup(packageName); if (pkg) { return; } } catch { - // empty + // Silently ignore errors when looking up the package + // This allows the function to continue with package creation }
20-26: Add comments to explain silent error handling for enum lookup.Similar to the previous comment, the silent error catching should be better documented.
// get current MessageType enum let enumType; try { enumType = messages.lookupEnum('MessageType'); } catch { - // empty + // Silently ignore errors when looking up MessageType enum + // This handles the case where the enum doesn't exist yet }
32-37: Add comments to explain silent error handling for package enum lookup.Same issue with silent error handling without proper documentation.
// get package MessageType enum let packageEnumType; try { packageEnumType = pkg.lookupEnum('MessageType'); } catch { - // empty + // Silently ignore errors when looking up MessageType enum in the package + // This handles the case where the package doesn't contain the enum }
5-9: Add input validation.The function doesn't validate its inputs. Adding checks for null or undefined parameters would make the function more robust.
export const loadDefinitions = async ( messages: Root, packageName: string, packageLoader: () => Definitions | Promise<Definitions>, ) => { + if (!messages) { + throw new Error('messages parameter is required'); + } + + if (!packageName) { + throw new Error('packageName parameter is required'); + } + + if (!packageLoader) { + throw new Error('packageLoader parameter is required'); + } // check if package already exists // ...
28-31: Consider adding error handling for packageLoader calls.The function doesn't handle errors from the packageLoader function explicitly. Consider adding a try-catch block around the packageLoader call for better error handling.
// load definitions - const packageMessages = await packageLoader(); - const pkg = messages.define(packageName, packageMessages); + try { + const packageMessages = await packageLoader(); + const pkg = messages.define(packageName, packageMessages); + + // get package MessageType enum + let packageEnumType; + try { + packageEnumType = pkg.lookupEnum('MessageType'); + } catch { + // Silently ignore errors when looking up MessageType enum in the package + // This handles the case where the package doesn't contain the enum + } + + // merge MessageType enums + if (enumType && packageEnumType) { + try { + // move values from nested enum to top level + Object.keys(packageEnumType.values).forEach(key => { + enumType.add(key, packageEnumType.values[key]); + }); + // remove nested enum + pkg.remove(packageEnumType); + } catch (e) { + // remove whole package on merge error + messages.remove(pkg); + + throw e; + } + } + } catch (error) { + throw new Error(`Failed to load package '${packageName}': ${error.message}`); + }Note: This is a more extensive refactoring that changes the flow of the function. Consider if this level of error handling is appropriate for your use case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/protobuf/src/index.ts(1 hunks)packages/protobuf/src/load-definitions.ts(1 hunks)packages/protobuf/tests/load-definitions.test.ts(1 hunks)packages/transport/src/transports/abstract.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: build-deploy
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: build
- GitHub Check: run-desktop-tests (@group=passphrase, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: build-web
- GitHub Check: transport-e2e-test
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
🔇 Additional comments (9)
packages/protobuf/src/index.ts (1)
22-22: LGTM! Clear export of the new loadDefinitions function.This provides proper access to the loadDefinitions functionality from the main protobuf module.
packages/transport/src/transports/abstract.ts (2)
1-1: LGTM! Import statement updated to include loadDefinitions.The import statement correctly adds the newly exported loadDefinitions function from the protobuf package.
347-349: LGTM! Well-implemented loadMessages method.This method provides a convenient way to extend message definitions in the transport layer. The implementation correctly leverages the loadDefinitions function and passes the current messages state.
The type definition
Parameters<typeof loadDefinitions>[2]accurately captures the third parameter of loadDefinitions, ensuring type safety.packages/protobuf/tests/load-definitions.test.ts (5)
5-15: LGTM! Well-structured test setup.The test suite is properly organized with a helper function to create a consistent Protobuf root for testing.
16-31: LGTM! Good test for merging MessageType enum.This test effectively verifies that new enum values are correctly merged with existing ones.
33-55: LGTM! Thorough error testing for enum merging.The test properly verifies error cases for both duplicate IDs and duplicate names, ensuring that the function fails as expected and cleans up properly.
57-73: LGTM! Good test for creating a new MessageType enum.This test correctly verifies the behavior when the MessageType enum is removed and then recreated.
75-89: LGTM! Good test for already loaded definitions.This test confirms that the packageLoader isn't called when the package already exists, which is an important optimization.
packages/protobuf/src/load-definitions.ts (1)
39-54: LGTM! Good error handling during enum merging.The implementation correctly handles errors during the merging process by removing the package if an error occurs. This ensures consistent state even in error cases.
Description
Cherry picked from
THPbranch.extend current protobuf definitions dynamically (load protobuf definitions when needed)