Skip to content

Conversation

@neithanmo
Copy link
Collaborator

@neithanmo neithanmo commented Apr 9, 2025

This PR implements a feature flag mechanism for Horizon support to prevent crashes. Key changes:

  • Added horizon.enabled = false configuration option in all relevant config files
  • Created a new HorizonConfig struct to properly manage the feature flag, and more settings if needed in the future.
  • Modified Horizon-related code to check the enabled flag before execution
  • Added conditional logic around database operations for Horizon
  • Updated tests to support the new configuration parameter
  • Renamed test function from tap_rav_test to test_tap_rav_v1 for clarity
  • Removed commented database migration code from start scripts

This change allows systems to run without Horizon support until full implementation is complete. The flag is set to false by default for safety.

Introduces a `horizon_enabled` boolean flag within `SenderAccountConfig`, populated from the main application configuration (`config.horizon.enabled`).

This flag gates Horizon-specific functionality within the TAP agent:

- In `SenderAccount`:
    - Database operations (fetching RAVs, querying/updating denylist) for `SenderType::Horizon` are now conditional on this flag being true.
    - The `todo!()` for querying unfinalized V2 transactions is now only relevant if Horizon is enabled.
    - A temporary check (`FIXME`) is added to `deny_sender_if_insolvent` to bypass denial logic for Horizon senders if the feature is disabled via config.
- In `SenderAccountsManager`:
    - Fetching pending V2 (Horizon) sender allocations is skipped if the flag is false.
    - The `new_receipts_watcher` task for V2/Horizon is only spawned if the flag is true.

This allows Horizon-related features, which may still be under development, to be effectively disabled via configuration, simplifying testing and allowing for incremental rollout without impacting existing functionality if Horizon support is not desired or ready.
fix(config): add missing config field
@coveralls
Copy link

coveralls commented Apr 9, 2025

Pull Request Test Coverage Report for Build 14360847977

Details

  • 22 of 60 (36.67%) changed or added relevant lines in 5 files are covered.
  • 3 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.01%) to 74.282%

Changes Missing Coverage Covered Lines Changed/Added Lines %
crates/tap-agent/src/agent/sender_accounts_manager.rs 18 19 94.74%
integration-tests/src/main.rs 0 1 0.0%
integration-tests/src/rav_tests.rs 0 1 0.0%
crates/tap-agent/src/agent/sender_account.rs 2 37 5.41%
Files with Coverage Reduction New Missed Lines %
integration-tests/src/rav_tests.rs 1 0.0%
crates/tap-agent/src/agent/sender_allocation.rs 2 76.32%
Totals Coverage Status
Change from base Build 14323791406: 0.01%
Covered Lines: 8876
Relevant Lines: 11949

💛 - Coveralls

@neithanmo neithanmo marked this pull request as ready for review April 9, 2025 15:30
@neithanmo
Copy link
Collaborator Author

i run the test locally and renames the test function to make it clear that the rav being tested is V1.
no crashes due to horizon missing tables anymore.

We need for sure to enable this local testnet integration test in our CI

Copy link
Collaborator

@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.

Thanks for this @neithanmo! It's really clear 💪

Copy link
Collaborator

@TypeLevelConsoli TypeLevelConsoli left a comment

Choose a reason for hiding this comment

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

That's wonderful. Clean and direct solution. Thank you so much!

@suchapalaver suchapalaver linked an issue Apr 9, 2025 that may be closed by this pull request
@neithanmo neithanmo merged commit 26a6f56 into main Apr 9, 2025
10 checks passed
@neithanmo neithanmo deleted the fix/v2_crash branch April 9, 2025 17:25
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.

agent crashing while cycling sender_allocation created

5 participants