Skip to content
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

feat(WIP): adding CreateAccount Operation #100

Merged
merged 8 commits into from
Jul 29, 2024
Merged

feat(WIP): adding CreateAccount Operation #100

merged 8 commits into from
Jul 29, 2024

Conversation

distractedm1nd
Copy link
Contributor

@distractedm1nd distractedm1nd commented Jul 27, 2024

References adr-002: #96

Copy link

coderabbitai bot commented Jul 27, 2024

Walkthrough

The recent code changes significantly enhance account operation management by introducing a common module that contains the Operation enum and HashchainEntry struct. These updates improve modularity and flexibility throughout the codebase. Additionally, the Sequencer has been refactored to adopt a more abstract and consistent approach to handling operations and hashchains, streamlining overall functionality and improving error handling.

Changes

File(s) Change Summary
src/common.rs Introduced Operation enum for account operations and HashchainEntry struct for hash chain entries.
src/node_types/sequencer.rs Replaced update_entry with process_operation, transitioned to using Operation, and refined error handling.
src/webserver.rs Renamed UpdateEntryJson to OperationInput, updated data types for operations and signatures.
src/storage.rs Removed ChainEntry, replacing it with HashchainEntry, and updated related method signatures.
src/utils.rs Removed is_revoked function, reflecting changes in public key revocation handling.
src/lib.rs, src/main.rs Added public module declaration for common, enhancing the modular structure of the library.
src/zk_snark.rs Revised create function to use HashchainEntry instead of ChainEntry, updating processing logic.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant WebServer
    participant Sequencer
    participant Storage

    User->>WebServer: Submit OperationInput
    WebServer->>Sequencer: Validate and queue update
    Sequencer->>Storage: Update hashchain with Operation
    Storage-->>Sequencer: Confirm update
    Sequencer-->>WebServer: Operation processed
    WebServer-->>User: Response
Loading

Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 0ba2bee and c65b99f.

Files selected for processing (8)
  • src/common.rs (1 hunks)
  • src/lib.rs (1 hunks)
  • src/main.rs (1 hunks)
  • src/node_types/sequencer.rs (8 hunks)
  • src/storage.rs (14 hunks)
  • src/utils.rs (2 hunks)
  • src/webserver.rs (5 hunks)
  • src/zk_snark.rs (2 hunks)
Files skipped from review as they are similar to previous changes (4)
  • src/common.rs
  • src/lib.rs
  • src/main.rs
  • src/zk_snark.rs
Additional comments not posted (25)
src/utils.rs (8)

Line range hint 14-17:
LGTM!

The parse_json_to_proof function correctly parses a JSON string into a Proof object and handles errors appropriately.


Line range hint 19-30:
LGTM!

The decode_public_key function correctly decodes a base64-encoded public key string into an Ed25519VerifyingKey and handles errors appropriately.


Line range hint 32-54:
LGTM!

The create_and_verify_snark function correctly creates and verifies a zkSNARK proof and handles errors appropriately.


Line range hint 56-78:
LGTM!

The validate_epoch function correctly validates an epoch using zkSNARK proof and handles errors appropriately.


Line range hint 80-86:
LGTM!

The SignedContent trait is correctly defined with appropriate methods for getting signature, plaintext, and public key.


Line range hint 88-103:
LGTM!

The verify_signature function correctly verifies the signature of a given signable item and handles errors appropriately.


Line range hint 105-198:
LGTM!

The tests module contains comprehensive tests for various utility functions and correctly tests their functionality.


1-1: Verify the impact of the removed is_revoked function.

The removal of the is_revoked function indicates a potential shift in how public key revocation is managed. Ensure that the revocation status of public keys is still verified appropriately elsewhere in the codebase.

src/webserver.rs (7)

38-43: LGTM!

The OperationInput struct correctly replaces the UpdateEntryJson struct and aligns with the new operation handling logic.


45-84: LGTM!

The implementation of the OperationInput struct, including the validate method, is correct and handles errors appropriately.


101-103: LGTM!

The UserKeyResponse struct correctly replaces Vec<ChainEntry> with Vec<HashchainEntry> and aligns with the new data handling logic.


119-128: LGTM!

The implementation of the SignedContent trait for the OperationInput struct is correct and handles errors appropriately.


Line range hint 173-181:
LGTM!

The update_entry function correctly updates its parameter type to accept Json<OperationInput> instead of Json<UpdateEntryJson> and aligns with the new operation handling logic.


Line range hint 183-194:
LGTM!

The get_hashchain function correctly retrieves the hashchain for a given user ID and handles errors appropriately.


Line range hint 196-204:
LGTM!

The get_commitment function correctly retrieves the current commitment of the IndexedMerkleTree and handles errors appropriately.

src/node_types/sequencer.rs (4)

40-41: LGTM!

The pending_entries field in the Sequencer struct correctly updates from Vec<IncomingEntry> to Vec<Operation> and aligns with the new operation handling logic.


250-259: LGTM!

The finalize_pending_entries function correctly processes all pending entries and returns the proofs, handling errors appropriately.


400-416: LGTM!

The create_new_entry function correctly creates a new OperationInput for creating an account and handles errors appropriately.


418-428: LGTM!

The create_update_entry function correctly creates a new OperationInput for adding an entry and handles errors appropriately.

src/storage.rs (6)

14-14: Import statement looks good.

The import statement for common::{HashchainEntry, Operation} is necessary for the changes made to the Operation enum and the new HashchainEntry struct.


32-41: Changes to the Database trait look good.

The method signatures for get_hashchain and update_hashchain have been updated to use HashchainEntry and Operation respectively. These changes align with the new data structures introduced.


Line range hint 272-298:
Test updates look good.

The test cases have been updated to use Operation and HashchainEntry. Ensure that the updated test cases correctly reflect the changes made to the data structures and method signatures.


528-542: Test case update looks good.

The test_update_hashchain test case has been updated to use Operation and HashchainEntry. Ensure that the test case correctly verifies the update_hashchain method's functionality with the new data structures.


Line range hint 107-114:
Ensure correct parsing in get_hashchain method.

The get_hashchain method has been updated to use HashchainEntry. Ensure that the parsing logic correctly handles the new structure.

Verification successful

The rg output shows that the get_hashchain method is defined in src/storage.rs. We need to verify that this method correctly deserializes the JSON string into a Vec<HashchainEntry>. Let's search for the complete implementation of the get_hashchain method to confirm the deserialization logic.


The get_hashchain method correctly uses serde_json::from_str to deserialize a JSON string into a Vec<HashchainEntry>. This indicates that the parsing logic is in place. To ensure completeness, we need to confirm that the HashchainEntry struct is properly defined with the necessary serde derives for JSON deserialization.


To ensure that the HashchainEntry struct supports JSON deserialization, we need to check for serde derives like Deserialize and Serialize.


The previous command resulted in a regex parse error. Let's correct the regex pattern and re-run the search for serde derives on the HashchainEntry struct.


Ensure correct parsing in get_hashchain method.

The get_hashchain method correctly uses serde_json::from_str to deserialize a JSON string into a Vec<HashchainEntry>. Additionally, the HashchainEntry struct has the necessary serde derives (Serialize, Deserialize) to support JSON deserialization.

  • src/storage.rs: The get_hashchain method correctly handles the deserialization of HashchainEntry.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify correct parsing of `HashchainEntry` in the `get_hashchain` method.

# Test: Search for the `HashchainEntry` usage. Expect: Correct parsing logic.
rg --type rust -A 5 $'HashchainEntry'

Length of output: 6598


Script:

#!/bin/bash
# Search for the complete implementation of the `get_hashchain` method in `src/storage.rs`
rg --type rust -A 20 'fn get_hashchain' src/storage.rs

Length of output: 2575


Script:

#!/bin/bash
# Search for the definition and serde derives of the `HashchainEntry` struct in `src/common.rs`
rg --type rust -A 10 'struct HashchainEntry' src/common.rs

Length of output: 390


Script:

#!/bin/bash
# Search for serde derives on the `HashchainEntry` struct in `src/common.rs`
rg --type rust -A 2 '#[derive' src/common.rs

Length of output: 117


Script:

#!/bin/bash
# Correct the regex pattern and search for serde derives on the `HashchainEntry` struct in `src/common.rs`
rg --type rust -A 2 '#\[derive' src/common.rs

Length of output: 692


187-201: Ensure correct handling of Operation and HashchainEntry in update_hashchain method.

The update_hashchain method has been updated to use Operation and HashchainEntry. Ensure that the logic correctly handles these new structures and updates the hashchain in the database.


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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

‼️ IMPORTANT
Auto-reply has been disabled for this repository in the CodeRabbit settings. The CodeRabbit bot will not respond to your replies unless it is explicitly tagged.

  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

src/node_types/sequencer.rs Outdated Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a 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, codebase verification and nitpick comments (1)
src/utils.rs (1)

Inconsistent usage of ChainEntry and HashChainEntry.

The codebase shows inconsistent usage of ChainEntry and HashChainEntry. Specifically:

  • ChainEntry is still present in src/storage.rs.
  • HashChainEntry is used in src/zk_snark.rs.

Please review and update the codebase to ensure consistent usage of HashChainEntry.

  • Files to review:
    • src/storage.rs
    • src/zk_snark.rs
Analysis chain

Line range hint 1-13:
Verify consistency of new data structure usage.

The removal of ChainEntry and possible shift to HashchainEntry should be consistent throughout the codebase.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of `HashchainEntry` instead of `ChainEntry`.

# Test: Search for `ChainEntry` in the codebase. Expect: No occurrences.
rg --type rust 'ChainEntry'

Length of output: 1025


Script:

#!/bin/bash
# Description: Verify the usage of `ChainEntry` and `HashChainEntry` in the codebase.

# Test: Search for all occurrences of `ChainEntry` and `HashChainEntry` in the codebase to verify consistency.
rg --type rust 'ChainEntry'
rg --type rust 'HashChainEntry'

Length of output: 1340

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

src/node_types/sequencer.rs Outdated Show resolved Hide resolved
src/node_types/sequencer.rs Outdated Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

src/node_types/sequencer.rs Outdated Show resolved Hide resolved
src/node_types/sequencer.rs Outdated Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

src/node_types/sequencer.rs Outdated Show resolved Hide resolved
src/node_types/sequencer.rs Outdated Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

src/node_types/sequencer.rs Show resolved Hide resolved
src/node_types/sequencer.rs Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

src/node_types/sequencer.rs Outdated Show resolved Hide resolved
src/node_types/sequencer.rs Outdated Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Outside diff range, codebase verification and nitpick comments (1)
src/node_types/sequencer.rs (1)

2-10: Unused Imports.

The imports for std::self, str::FromStr, and time::Duration are not used in the code.

Consider removing these unused imports to keep the code clean and maintainable.

src/node_types/sequencer.rs Show resolved Hide resolved
src/node_types/sequencer.rs Show resolved Hide resolved
src/node_types/sequencer.rs Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

src/node_types/sequencer.rs Show resolved Hide resolved
src/node_types/sequencer.rs Show resolved Hide resolved
@distractedm1nd distractedm1nd merged commit 16dfbf7 into main Jul 29, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants