Skip to content

Add get endorsees for plaintext chain for wildcat#577

Merged
zupzup merged 1 commit into
masterfrom
561-add-get-endorsees-for-plaintext
Jul 7, 2025
Merged

Add get endorsees for plaintext chain for wildcat#577
zupzup merged 1 commit into
masterfrom
561-add-get-endorsees-for-plaintext

Conversation

@zupzup
Copy link
Copy Markdown
Collaborator

@zupzup zupzup commented Jul 7, 2025

User description

📝 Description

  • Add get endorsees for plaintext chain for wildcat

Relates to #561


✅ Checklist

Please ensure the following tasks are completed before requesting a review:

  • My code adheres to the coding guidelines of this project.
  • I have run cargo fmt.
  • I have run cargo clippy.
  • I have added or updated tests (if applicable).
  • All CI/CD steps were successful.
  • I have updated the documentation (if applicable).
  • I have checked that there are no console errors or warnings.
  • I have verified that the application builds without errors.
  • I've described the changes made to the API. (modification, addition, deletion).

🚀 Changes Made

See above.


💡 How to Test

Please provide clear instructions on how reviewers can test your changes:

  1. cargo test

🤝 Related Issues

List any related issues, pull requests, or discussions:


📋 Review Guidelines

Please focus on the following while reviewing:

  • Does the code follow the repository's contribution guidelines?
  • Are there any potential bugs or performance issues?
  • Are there any typos or grammatical errors in the code or comments?

PR Type

Enhancement


Description

  • Add get_endorsees_from_chain_with_plaintext function for extracting endorsees

  • Implement get_holder method for BillBlockPlaintextWrapper

  • Add test coverage for plaintext chain endorsee extraction

  • Version bump to 0.4.2


Changes diagram

flowchart LR
  A["BillBlockPlaintextWrapper"] --> B["get_holder method"]
  B --> C["Extract holder info by op_code"]
  C --> D["get_endorsees_from_chain_with_plaintext"]
  D --> E["Return endorsees list"]
Loading

Changes walkthrough 📝

Relevant files
Enhancement
chain.rs
Add endorsee extraction for plaintext chains                         

crates/bcr-ebill-core/src/blockchain/bill/chain.rs

  • Add get_holder method to extract holder information from different
    block types
  • Implement get_endorsees_from_chain_with_plaintext function to collect
    endorsees
  • Add import for HolderFromBlock type
  • Include test case for endorsee extraction functionality
  • +78/-1   
    Documentation
    CHANGELOG.md
    Document endorsee extraction feature                                         

    CHANGELOG.md

    • Add entry for version 0.4.2 with endorsee extraction feature
    +4/-0     
    Configuration changes
    Cargo.toml
    Version bump to 0.4.2                                                                       

    Cargo.toml

    • Bump version from 0.4.1 to 0.4.2
    +1/-1     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @qodo-code-review
    Copy link
    Copy Markdown

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis 🔶

    561 - Partially compliant

    Compliant requirements:

    • Add functionality to share a bill with an external party (partial - endorsee extraction)

    Non-compliant requirements:

    • Add block_data_hash field to Block (identity and company)
    • Serialize the full Bill Chain
    • Encrypt the chain with external party's public key
    • Sign the blob and send to external party

    Requires further human verification:

    • Enable verification that outer/inner hashes and signature are correct via Nostr

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Logic Gap

    The get_endorsees_from_chain_with_plaintext function skips Issue blocks but doesn't handle all possible block types. Some block types may not have holders and could cause silent failures or incomplete endorsee lists.

    pub fn get_endorsees_from_chain_with_plaintext(
        chain_with_plaintext: &[BillBlockPlaintextWrapper],
    ) -> Vec<BillParticipant> {
        let mut result: Vec<BillParticipant> = vec![];
        // iterate from the front to the back, collecting all endorsement blocks
        for block_wrapper in chain_with_plaintext.iter() {
            // we ignore issue blocks, since we are only interested in endorsements
            if block_wrapper.block.op_code == BillOpCode::Issue {
                continue;
            }
            if let Ok(Some(holder_from_block)) = block_wrapper.get_holder() {
                let holder = holder_from_block.holder;
                result.push(holder.into());
            }
        }
    
        result
    }
    Error Handling

    The get_holder method uses borsh::from_slice which can fail, but in get_endorsees_from_chain_with_plaintext these errors are silently ignored with if let Ok(Some(...)), potentially masking data corruption issues.

    if let Ok(Some(holder_from_block)) = block_wrapper.get_holder() {
        let holder = holder_from_block.holder;
        result.push(holder.into());
    }

    Copy link
    Copy Markdown
    Contributor

    Copilot AI left a comment

    Choose a reason for hiding this comment

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

    Pull Request Overview

    This PR adds functionality to extract and list holders (endorsees) from a plaintext bill chain and updates the project version.

    • Introduces BillBlockPlaintextWrapper::get_holder to uniformly retrieve holder, signer, and signatory data.
    • Adds get_endorsees_from_chain_with_plaintext to collect all recipients of applicable operations in a chain.
    • Bumps workspace version and records the new feature in the changelog.

    Reviewed Changes

    Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

    File Description
    crates/bcr-ebill-core/src/blockchain/bill/chain.rs Added get_holder, get_endorsees_from_chain_with_plaintext, and associated test assertion.
    Cargo.toml Updated workspace version to 0.4.2
    CHANGELOG.md Added entry for 0.4.2 noting endorsees logic
    Comments suppressed due to low confidence (1)

    crates/bcr-ebill-core/src/blockchain/bill/chain.rs:202

    • [nitpick] The name get_endorsees_from_chain_with_plaintext implies only endorsement recipients, but it also collects holders from mint, sell, and recourse operations. Consider renaming or updating the doc comment to clarify the full set of included operations.
    pub fn get_endorsees_from_chain_with_plaintext(
    

    Comment thread crates/bcr-ebill-core/src/blockchain/bill/chain.rs
    Comment thread crates/bcr-ebill-core/src/blockchain/bill/chain.rs
    @qodo-code-review
    Copy link
    Copy Markdown

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Propagate deserialization errors properly

    The function silently ignores errors from get_holder() calls, which could mask
    critical deserialization failures. Consider propagating errors or at least
    logging them to ensure data integrity issues are not overlooked.

    crates/bcr-ebill-core/src/blockchain/bill/chain.rs [202-219]

     pub fn get_endorsees_from_chain_with_plaintext(
         chain_with_plaintext: &[BillBlockPlaintextWrapper],
    -) -> Vec<BillParticipant> {
    +) -> Result<Vec<BillParticipant>> {
         let mut result: Vec<BillParticipant> = vec![];
         // iterate from the front to the back, collecting all endorsement blocks
         for block_wrapper in chain_with_plaintext.iter() {
             // we ignore issue blocks, since we are only interested in endorsements
             if block_wrapper.block.op_code == BillOpCode::Issue {
                 continue;
             }
    -        if let Ok(Some(holder_from_block)) = block_wrapper.get_holder() {
    +        if let Some(holder_from_block) = block_wrapper.get_holder()? {
                 let holder = holder_from_block.holder;
                 result.push(holder.into());
             }
         }
     
    -    result
    +    Ok(result)
     }
    • Apply / Chat
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion correctly identifies that the if let Ok(...) pattern silently ignores potential deserialization errors from get_holder(), and the proposed change to propagate the error using Result and the ? operator significantly improves the function's robustness.

    Medium
    • More

    @codecov
    Copy link
    Copy Markdown

    codecov Bot commented Jul 7, 2025

    Codecov Report

    Attention: Patch coverage is 87.50000% with 6 lines in your changes missing coverage. Please review.

    Files with missing lines Patch % Lines
    crates/bcr-ebill-core/src/blockchain/bill/chain.rs 87.50% 6 Missing ⚠️

    📢 Thoughts on this report? Let us know!

    @zupzup zupzup merged commit 945a756 into master Jul 7, 2025
    7 checks passed
    @zupzup zupzup deleted the 561-add-get-endorsees-for-plaintext branch July 7, 2025 10:56
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    3 participants