-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Requests to access node use CCF encoding from now on #53
Requests to access node use CCF encoding from now on #53
Conversation
4805623
to
1ffc41a
Compare
Can u review it? @peterargue |
I got it working in my original PR against testnet. needed to make the following changes
After those changes, it was able to index using my PR. @illia-malachyn I think the issues you had were related to updating to library versions that do not support testnet which is running |
The indexer is working even without these changes. I added the |
I double-checked this PR. The indexer works well. All 3 commits are required. |
1734640
to
0a062b3
Compare
0a062b3
to
ae4023f
Compare
WalkthroughThe recent changes enhance the encoding and processing of events and transactions by introducing the Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant Client
participant AccessService
participant Entities
Client->>AccessService: BlockEvents()
AccessService->>Entities: GetEventsForBlockIDsRequest(EventEncodingVersion)
Entities-->>AccessService: Event Data
AccessService-->>Client: Event Data
Client->>AccessService: Transaction()
AccessService->>Entities: GetTransactionRequest(EventEncodingVersion)
Entities-->>AccessService: Transaction Data
AccessService-->>Client: Transaction Data
Client->>AccessService: TransactionResult()
AccessService->>Entities: GetTransactionByIndexRequest(EventEncodingVersion)
Entities-->>AccessService: Transaction Result Data
AccessService-->>Client: Transaction Result Data
Assessment against linked issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
state/process.go (2)
Line range hint
307-324
: Consider refactoring to reduce complexity and improve readability.The nested loops and conditionals in the
processBlock
function make it hard to follow and maintain. Consider breaking down the function into smaller, more manageable functions. Each segment handling a specific part of the block processing (e.g., transaction fetching, result processing, error handling) could be a separate function.
Line range hint
366-391
: Add documentation to clarify the purpose and behavior ofuseSlowPath
.The
useSlowPath
function uses specific error codes and messages to determine the control flow. Adding detailed documentation about why each error leads to the slow path would help maintainers understand the decision logic. Additionally, consider defining constants for error messages to avoid magic strings in the code.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
go.mod
is excluded by!**/*.mod
go.sum
is excluded by!**/*.sum
Files selected for processing (4)
- access/access.go (6 hunks)
- state/convert.go (3 hunks)
- state/process.go (2 hunks)
- testnet-clone.json (1 hunks)
Additional comments not posted (4)
testnet-clone.json (1)
21-28
: The updatedsporks
configuration with the newroot_block
andversion
values seems correct. Ensure that these values are consistent with the intended spork updates for the testnet environment.state/convert.go (2)
7-9
: The addition of new imports forcadence
,ccf
, andjsoncdc
is appropriate for supporting the new CCF encoding. Ensure that these libraries are compatible with the rest of the project dependencies.
Line range hint
186-214
: The implementation ofdecodeEvent
anddecodePayload
to handle CCF encoded payloads is a crucial part of the solution to the problem described in Issue #41. It's important to ensure that the error handling and logging within these functions are robust and provide enough information for debugging.access/access.go (1)
168-170
: The addition of theEventEncodingVersion
field set toentities.EventEncodingVersion_CCF_V0
in various client methods is a significant change. This aligns with the PR's objective to standardize the event encoding to CCF format. Ensure that this new encoding version is supported and correctly implemented in all related backend services and that it does not break existing functionalities.Also applies to: 390-391, 411-413, 434-435, 460-461, 481-482
Closes #41
Summary by CodeRabbit
New Features
Bug Fixes
Configuration
testnet-clone.json
with new spork configuration for better network synchronization.