fix: ensure only one from_block field is set in subscribeToTransactionsWithProofs #2537
Conversation
WalkthroughThe update refines the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Function as subscribeToTransactionsWithProofs
participant RequestBuilder as Request
Caller->>Function: Call subscribeToTransactionsWithProofs(options)
Function->>Function: Check if options.fromBlockHash exists?
alt fromBlockHash exists
Function->>RequestBuilder: Set request.fromBlockHash
else
Function->>Function: Check if options.fromBlockHeight is explicitly provided
alt fromBlockHeight provided
Function->>RequestBuilder: Set request.fromBlockHeight
end
end
Function->>Caller: Return TransactionsWithProofsRequest
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 review |
✅ Actions performedReview triggered.
|
![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 (1)
packages/js-dapi-client/lib/methods/core/subscribeToTransactionsWithProofsFactory.js (1)
86-91: Consider documenting the precedence behavior.While the implementation correctly prioritizes
fromBlockHashoverfromBlockHeight, it might be helpful to document this behavior in the JSDoc for clarity./** * @typedef {object} subscribeToTransactionsWithProofsOptions * @property {string} [fromBlockHash] - Specifies block hash to start syncing from * @property {number} [fromBlockHeight] - Specifies block height to start syncing from + * @note If both fromBlockHash and fromBlockHeight are provided, fromBlockHash takes precedence * @property {number} [count=0] - Number of blocks to sync, * if set to 0 syncing is continuously sends new data as well */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/js-dapi-client/lib/methods/core/subscribeToTransactionsWithProofsFactory.js(2 hunks)
🔇 Additional comments (2)
packages/js-dapi-client/lib/methods/core/subscribeToTransactionsWithProofsFactory.js (2)
42-42: Good fix: Improved validation logic for fromBlockHeight.The condition now correctly checks if
fromBlockHeightis explicitly included in the options before validating its value. This prevents throwing an error when the property is not defined, addressing part of the original bug where unintended serialization would occur.
62-70: Well implemented: Prioritizing fromBlockHash over fromBlockHeight.This change correctly implements the priority handling logic by first checking for
fromBlockHashand only consideringfromBlockHeightwhen the hash is not provided. This ensures mutual exclusivity between these fields in the Protobuf message, preventing the gRPC error mentioned in the PR description.The handling of the hash value is also robust, with proper checks for Buffer type or conversion from hex string.
|
I see now that there is a specific test case that does this (block height 0, but with a block hash). It doesn't really make sense to me that you would pass both parameters in the same call, but maybe there is a reason? 🤔 EDIT: actually the fact that core.proto defines those params using a Based on some testing with gRPCurl,
|
| if ('fromBlockHeight' in options && options.fromBlockHeight === 0) { | ||
| throw new DAPIClientError('Invalid argument: minimum value for `fromBlockHeight` is 1'); | ||
| } |
There was a problem hiding this comment.
If we added something like this, it could prevent confusion and unexpected behavior. I guess that would be a breaking change though.
| if ('fromBlockHeight' in options && options.fromBlockHeight === 0) { | |
| throw new DAPIClientError('Invalid argument: minimum value for `fromBlockHeight` is 1'); | |
| } | |
| if ('fromBlockHash' in options && 'fromBlockHeight' in options) { | |
| throw new DAPIClientError('Only one of fromBlockHash or fromBlockHeight may be provided'); | |
| } | |
| if ('fromBlockHeight' in options && options.fromBlockHeight === 0) { | |
| throw new DAPIClientError('Invalid argument: minimum value for `fromBlockHeight` is 1'); | |
| } |
There was a problem hiding this comment.
0 is default value for integer in protobuf event if this field is omitted.
There was a problem hiding this comment.
I found/fixed an issue in the code I was trying to use dapi-client with. However, dapi-client's handling of these options is still misleading. Someone should only provide either a height or a hash, but it's possible to provide both and it's not very obvious that the height is ignored in that case. Wouldn't it make sense to notify the dev they should only be passing one of the two so they don't waste time debugging expected behavior?
thephez
force-pushed
the
fix/subscribeToTransactionsWithProofs-oneof
branch
from
318924e to
c8fe881
Compare
| if ('fromBlockHeight' in options && options.fromBlockHeight === 0) { | ||
| throw new DAPIClientError('Invalid argument: minimum value for `fromBlockHeight` is 1'); | ||
| } |
There was a problem hiding this comment.
0 is default value for integer in protobuf event if this field is omitted.
Issue being fixed or feature implemented
This PR resolves a bug in dapi-client where providing
fromBlockHashtosubscribeToTransactionsWithProofs()would still result in a gRPC error due to unintended Protobuf serialization offromBlockHeight = 0.What was done?
fromBlockHeightwas explicitly passed and is zero.fromBlockHeightis only used whenfromBlockHashis not present.How Has This Been Tested?
CI
Breaking Changes
N/A
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit