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

refactor: aggregate ravs depending on allocation type #596

Merged
merged 12 commits into from
Feb 5, 2025

Conversation

gusinacio
Copy link
Member

@gusinacio gusinacio commented Jan 31, 2025

  • Create NetworkVersion trait used to define what kind of allocation we are handling.
  • Add Generic for TapContext over NetworkVersion.
  • Add Legacy and Horizon that implement NetworkVersion
  • Wrap allocation Id address with new AllocationId enum.
  • Create TapManager version depending on the allocation id variant.
  • Implement aggregation within NetworkVersion trait.
  • Add aggregator_v2 connection for SenderAccount.

TODO for future PRs:

  • Implement all unimplemented traits for database queries.
  • Test more extensively different types of allocations running.

Note

tap-agent is not well organized and we have a few files with 1k+ lines. Suggestions on how to organize are accepted.

Copy link
Contributor

github-actions bot commented Jan 31, 2025

Pull Request Test Coverage Report for Build 13145110259

Details

  • 155 of 239 (64.85%) changed or added relevant lines in 7 files are covered.
  • 15 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.7%) to 77.999%

Changes Missing Coverage Covered Lines Changed/Added Lines %
crates/tap-agent/src/agent/sender_allocation.rs 31 32 96.88%
crates/tap-agent/src/tap/context/rav.rs 0 6 0.0%
crates/tap-agent/src/tap/context/receipt.rs 12 18 66.67%
crates/tap-agent/src/tap/context.rs 19 49 38.78%
crates/tap-agent/src/agent/sender_account.rs 56 97 57.73%
Files with Coverage Reduction New Missed Lines %
crates/service/src/tap/checks/deny_list_check.rs 2 94.4%
crates/watcher/src/lib.rs 4 92.71%
crates/tap-agent/src/agent/sender_account.rs 9 77.41%
Totals Coverage Status
Change from base Build 13141469414: -0.7%
Covered Lines: 7158
Relevant Lines: 9177

💛 - Coveralls

@gusinacio gusinacio force-pushed the gustavo/aggregate-horizon-receipts branch from 70c9a9a to 35046f6 Compare February 4, 2025 14:28
@gusinacio gusinacio changed the title refactor: add allocation type refactor: aggregate ravs depending on allocation type Feb 4, 2025
@gusinacio gusinacio force-pushed the gustavo/aggregate-horizon-receipts branch 5 times, most recently from e208f23 to 355b891 Compare February 4, 2025 19:44
@gusinacio gusinacio marked this pull request as ready for review February 4, 2025 19:58
@gusinacio gusinacio force-pushed the gustavo/aggregate-horizon-receipts branch from 47ed6ec to d1c87d4 Compare February 4, 2025 21:23
crates/tap-agent/src/agent/sender_account.rs Show resolved Hide resolved
crates/tap-agent/src/tap/context.rs Outdated Show resolved Hide resolved
crates/tap-agent/src/tap/context.rs Outdated Show resolved Hide resolved
crates/tap-agent/src/tap/context/receipt.rs Show resolved Hide resolved
crates/tap-agent/src/tap/context.rs Outdated Show resolved Hide resolved
@gusinacio gusinacio force-pushed the gustavo/aggregate-horizon-receipts branch from 03cda08 to 21a37af Compare February 5, 2025 16:10
Copy link
Contributor

@suchapalaver suchapalaver left a comment

Choose a reason for hiding this comment

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

@gusinacio Thanks so much this looks great and thank you for all the amazing knowledge-sharing in the docs and comments! Just a couple of small things I caught and then good to go 🚀 !

crates/tap-agent/src/agent/sender_account.rs Outdated Show resolved Hide resolved
crates/tap-agent/src/agent/sender_account.rs Outdated Show resolved Hide resolved
crates/tap-agent/src/agent/sender_account.rs Show resolved Hide resolved
crates/tap-agent/src/tap/context.rs Outdated Show resolved Hide resolved
crates/tap-agent/src/tap/context.rs Outdated Show resolved Hide resolved
crates/tap-agent/src/tap/context.rs Outdated Show resolved Hide resolved
crates/tap-agent/src/tap/context.rs Outdated Show resolved Hide resolved
crates/tap-agent/src/tap/context/receipt.rs Show resolved Hide resolved
Copy link
Contributor

@suchapalaver suchapalaver left a comment

Choose a reason for hiding this comment

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

Awesome! 🙏

@gusinacio gusinacio merged commit 3d90f29 into main Feb 5, 2025
10 checks passed
@gusinacio gusinacio deleted the gustavo/aggregate-horizon-receipts branch February 5, 2025 18:26
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