Skip to content

Conversation

@cce
Copy link
Contributor

@cce cce commented Sep 17, 2025

Summary

VerifiedTransactionCache contains these interface methods, introduced in #1757

Add(txgroup []transactions.SignedTxn, groupCtx *GroupContext)
AddPayset(txgroup [][]transactions.SignedTxn, groupCtx []*GroupContext)

But since GroupContext already contains the txgroup, the first arg is redundant.

Test Plan

  • Add panic assertions that these two values are always equal
  • Remove first arg from interface methods and update code
  • Existing tests should pass

@codecov
Copy link

codecov bot commented Sep 17, 2025

Codecov Report

❌ Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 50.64%. Comparing base (8245ed1) to head (5808939).
⚠️ Report is 6 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
data/transactions/verify/verifiedTxnCache.go 0.00% 6 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6444      +/-   ##
==========================================
- Coverage   50.84%   50.64%   -0.20%     
==========================================
  Files         664      657       -7     
  Lines      111455   111364      -91     
==========================================
- Hits        56665    56405     -260     
- Misses      51924    52080     +156     
- Partials     2866     2879      +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cce cce marked this pull request as ready for review September 17, 2025 18:39
@cce cce requested review from algorandskiy and Copilot September 18, 2025 18:50
Copy link
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 removes redundant transaction group parameters from the VerifiedTransactionCache interface methods. Since GroupContext already contains the transaction group via its signedGroupTxns field, passing the transaction group separately was unnecessary duplication.

  • Removed the first txgroup parameter from Add and AddPayset methods in the interface
  • Updated all implementations and callers to use only the GroupContext parameter
  • Simplified the internal add method to extract transaction group from GroupContext

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
data/transactions/verify/verifiedTxnCache.go Updated interface definition and implementation to remove redundant txgroup parameters
data/transactions/verify/txnBatch.go Updated calls to cache methods and removed now-unused verifiedTxnGroups slice
data/transactions/verify/txn.go Updated cache method calls to pass only GroupContext
data/transactions/verify/verifiedTxnCache_test.go Updated test code to use new method signatures and properly prepare GroupContext
data/txHandler_test.go Updated mock implementation to match new interface

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@cce cce requested a review from jannotti September 19, 2025 14:45
@gmalouf gmalouf merged commit 5da907f into algorand:master Sep 19, 2025
43 checks passed
algorandskiy pushed a commit to algorandskiy/go-algorand that referenced this pull request Sep 25, 2025
@cce cce deleted the groupctx-cache-simplify branch October 1, 2025 19:16
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