Skip to content

Conversation

@jannotti
Copy link
Contributor

@jannotti jannotti commented Mar 27, 2025

Summary

Some cleanups I ran into while doing tx.Access. Wanted to make this PR first because that PR risks being big anyway.

transaction.WellFormed() was huge. Move each transaction's implementation into the file that defined the transaction type's field.

transaction.Alive() is better thought of as a method on block. It's the block that decides if the transaction is Alive. By moving it there, we remove an interface that was only introduced to fix the circular dependency caused by putting in the "wrong" place.

transaction.MatchAddress is rewritten to avoid accumulating a slice of addresses just to call slice.Contains on it.

AddressTxns is now TxnsFrom since its callers (stateproof REST endpoints) really only wanted to inspect the Sender anyway. They wanted the transactions sent by a particular address, not any transaction related to that address.

We should consider making MatchAddress work properly for additional types like acfg and freeze, but it's only used to decide what transactions to return from pending when an address filter is given.

Made some changes to how we turn evaldeltas into consumable json. The pending endpoint and the dry-run code were both doing, in slightly different ways. I'll need to modify it for tx.Access, so I wanted to clean it up now.

Test Plan

No meaningful tests should have been removed, only moved, hopefully to more meaningful places. The biggest motion was creating /transaction/<txType>_test.go in cases where it didn't exist, to hold the associated WellFormedness checks.

(By accident of history, payment_test was doing some general transaction tests, because that was basically the only transaction at one time.)

Some cleanups I ran into while doing tx.Access.  Wanted to make this
PR first because that PR risks being big anyway.

transaction.WellFormed() was huge. Move each transaction's
implementation into the file that defined the transaction type's
field.

transaction.Alive() is better thought of as a method on block.  It's
the block that decides if the transaction is Alive. By moving it
there, we remove an interface that was only introduced to fix the
circular dependency caused by putting in the "wrong" place.
@jannotti jannotti self-assigned this Mar 27, 2025
@jannotti jannotti force-pushed the refactor-txn-tests branch from 9b3c0bf to ab82579 Compare March 27, 2025 18:34
@jannotti jannotti force-pushed the refactor-txn-tests branch from ab82579 to 5dedcf9 Compare March 27, 2025 19:06
@codecov
Copy link

codecov bot commented Mar 27, 2025

Codecov Report

Attention: Patch coverage is 71.72131% with 69 lines in your changes missing coverage. Please review.

Project coverage is 51.72%. Comparing base (55011f9) to head (7aa72cb).
Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
data/transactions/transaction.go 35.13% 24 Missing ⚠️
data/transactions/application.go 69.49% 10 Missing and 8 partials ⚠️
data/bookkeeping/block.go 50.00% 10 Missing and 3 partials ⚠️
daemon/algod/api/server/v2/handlers.go 0.00% 8 Missing ⚠️
data/ledger.go 0.00% 4 Missing ⚠️
daemon/algod/api/server/v2/utils.go 89.47% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6287      +/-   ##
==========================================
+ Coverage   51.70%   51.72%   +0.02%     
==========================================
  Files         647      649       +2     
  Lines       86874    86849      -25     
==========================================
+ Hits        44917    44922       +5     
+ Misses      39100    39058      -42     
- Partials     2857     2869      +12     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jannotti jannotti marked this pull request as ready for review March 27, 2025 19:39
algorandskiy
algorandskiy previously approved these changes Mar 28, 2025
Copy link
Contributor

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

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

In general looks good, not sure about getting rid First/Last getters

Simplifying before tx.Access work
@jannotti jannotti merged commit 160bcf3 into algorand:master Mar 31, 2025
19 checks passed
cce pushed a commit to cce/go-algorand that referenced this pull request May 29, 2025
@jannotti jannotti deleted the refactor-txn-tests branch July 15, 2025 16:22
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