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(settings): refactor genesis module #794

Merged
merged 1 commit into from
Oct 10, 2023
Merged

Conversation

glevco
Copy link
Contributor

@glevco glevco commented Sep 28, 2023

Depends on #787

Motivation

This PR not only removes any usage of global settings from the genesis module, but it also substitutes all global variables defined in this module by functions. It also includes some simplifications in code using that module.

Acceptance Criteria

  • Remove BLOCK_GENESIS, TX_GENESIS1, TX_GENESIS2, GENESIS, GENESIS_HASHES, GENESIS_HASH global variables and implement, respectively, get_genesis_block(), get_genesis_tx1(), get_genesis_tx2(), get_all_genesis(), get_all_genesis_hashes(), get_genesis_hash(). As these are not shared instances anymore, it's safe to manipulate the returned instance in tests, for example.
  • Simplify get_genesis_hash() and is_genesis() implementations.
  • Remove _get_genesis_transactions_unsafe() function as after all refactors it was only used in one place.
  • Change CliBuilder to use the existing get_genesis_short_hash() function.
  • Removed height_index.BLOCK_GENESIS_ENTRY global variable and refactored it into HeightIndex.get_genesis_block_entry().
  • Update all code using removed global variables.

Checklist

  • If you are requesting a merge into master, confirm this code is production-ready and can be included in future releases as soon as it gets merged

@glevco glevco self-assigned this Sep 28, 2023
@glevco glevco marked this pull request as ready for review September 28, 2023 23:04
@glevco glevco changed the base branch from master to refactor/hathor-manager-settings September 29, 2023 13:39
@codecov
Copy link

codecov bot commented Sep 29, 2023

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (c700dc5) 84.65% compared to head (96b93db) 84.69%.
Report is 1 commits behind head on master.

❗ Current head 96b93db differs from pull request most recent head 88ef27f. Consider uploading reports for the commit 88ef27f to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #794      +/-   ##
==========================================
+ Coverage   84.65%   84.69%   +0.04%     
==========================================
  Files         264      264              
  Lines       22034    22019      -15     
  Branches     3369     3364       -5     
==========================================
- Hits        18652    18649       -3     
+ Misses       2726     2717       -9     
+ Partials      656      653       -3     
Files Coverage Δ
hathor/builder/builder.py 91.02% <ø> (ø)
hathor/builder/cli_builder.py 74.35% <100.00%> (-0.14%) ⬇️
hathor/indexes/base_index.py 100.00% <100.00%> (ø)
hathor/indexes/height_index.py 96.61% <100.00%> (ø)
hathor/indexes/memory_height_index.py 83.33% <100.00%> (ø)
hathor/indexes/memory_info_index.py 91.83% <100.00%> (+0.17%) ⬆️
hathor/indexes/rocksdb_height_index.py 94.17% <100.00%> (+0.05%) ⬆️
hathor/p2p/utils.py 74.10% <100.00%> (+0.23%) ⬆️
hathor/simulator/simulator.py 94.61% <100.00%> (-0.04%) ⬇️
hathor/transaction/base_transaction.py 94.16% <100.00%> (ø)
... and 4 more

... and 6 files with indirect coverage changes

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

jansegre
jansegre previously approved these changes Oct 3, 2023
hathor/transaction/genesis.py Show resolved Hide resolved
@glevco glevco force-pushed the refactor/hathor-manager-settings branch 2 times, most recently from c700dc5 to 85a3bfc Compare October 9, 2023 18:29
Base automatically changed from refactor/hathor-manager-settings to master October 9, 2023 18:30
@msbrogli msbrogli dismissed jansegre’s stale review October 9, 2023 18:30

The base branch was changed.

hathor/transaction/genesis.py Outdated Show resolved Hide resolved
hathor/transaction/genesis.py Outdated Show resolved Hide resolved
hathor/transaction/genesis.py Outdated Show resolved Hide resolved
hathor/transaction/genesis.py Outdated Show resolved Hide resolved
hathor/transaction/genesis.py Outdated Show resolved Hide resolved
hathor/transaction/genesis.py Outdated Show resolved Hide resolved
hathor/transaction/genesis.py Outdated Show resolved Hide resolved
hathor/transaction/genesis.py Outdated Show resolved Hide resolved
@glevco
Copy link
Contributor Author

glevco commented Oct 9, 2023

@msbrogli @jansegre as we discussed, in a86c175 I made the following changes:

  • In HathorSettings:
    • Rename GENESIS_TIMESTAMP to GENESIS_BLOCK_TIMESTAMP.
    • Add GENESIS_TX1_TIMESTAMP and GENESIS_TX2_TIMESTAMP calculated properties. This simplifies some use cases of the removed genesis module functions.
  • In genesis module:
    • Rename get_genesis_hash() to get_representation_for_all_genesis().
    • Remove get_genesis_block(), get_genesis_tx1(), get_genesis_tx2(), and get_all_genesis().
  • In TransactionStorage:
    • Remove _get_genesis_from_settings().
    • Add _construct_genesis_block(), _construct_genesis_tx1(), and _construct_genesis_tx2().
  • In test_tx_storage I had to remove part of a test, since it doesn't make sense anymore.
  • In tests/utils remove get_min_timestamp().

@msbrogli msbrogli merged commit 88ef27f into master Oct 10, 2023
@msbrogli msbrogli deleted the refactor/genesis branch October 10, 2023 20:31
This was referenced Oct 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants