-
Notifications
You must be signed in to change notification settings - Fork 235
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
Problem: bulk-add-genesis-account is not used in testground #1582
Conversation
Solution: - extract standalone changes from crypto-org-chain#1575 - use a simple and deterministic way to generate test accounts
WalkthroughThe pull request includes updates to the Changes
Possibly related PRs
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1582 +/- ##
===========================================
+ Coverage 15.24% 36.12% +20.88%
===========================================
Files 67 97 +30
Lines 4874 7725 +2851
===========================================
+ Hits 743 2791 +2048
- Misses 4037 4585 +548
- Partials 94 349 +255 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems validator doesnt generate when validator_generate_load is false?
WDYM? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- testground/benchmark/benchmark/peer.py (6 hunks)
- testground/benchmark/benchmark/types.py (1 hunks)
- testground/benchmark/benchmark/utils.py (4 hunks)
Additional context used
Ruff
testground/benchmark/benchmark/utils.py
4-4:
itertools.dropwhile
imported but unusedRemove unused import
(F401)
4-4:
itertools.takewhile
imported but unusedRemove unused import
(F401)
GitHub Check: Lint python
testground/benchmark/benchmark/utils.py
[failure] 4-4:
./testground/benchmark/benchmark/utils.py:4:1: F401 'itertools.dropwhile' imported but unused
[failure] 4-4:
./testground/benchmark/benchmark/utils.py:4:1: F401 'itertools.takewhile' imported but unused
Additional comments not posted (8)
testground/benchmark/benchmark/types.py (3)
8-13
: LGTM!The
Balance
class is well-structured and provides a clear representation of a balance with amount and denomination. The use ofpydantic
'sBaseModel
ensures proper validation and serialization. The__str__
method is a nice addition for convenient string formatting.
18-18
: Improved data modeling with thecoins
attribute.The change from
balance
tocoins
is a significant improvement in the data modeling of theGenesisAccount
class. It allows for a more flexible and accurate representation of account balances by supporting multiple denominations. The use of a list ofBalance
instances provides a structured and type-safe approach.
18-18
: Verify the usage of thecoins
attribute in the codebase.Ensure that all references to the
balance
attribute are updated to use the newcoins
attribute and handle the list ofBalance
instances appropriately.Run the following script to verify the usage of the
coins
attribute:testground/benchmark/benchmark/utils.py (3)
14-15
: LGTM!Defining the Cronos address prefix as a constant is a good practice. It improves code readability and maintainability.
98-100
: LGTM!The
eth_to_bech32
function provides a useful utility for converting Ethereum addresses to Bech32 format. The implementation looks correct and follows the Bech32 encoding process.
137-142
: LGTM!The
gen_account
function provides a deterministic way to generate test accounts based on a global sequence and an index. This is useful for testing and benchmarking purposes. Reserving index 0 for a validator account is a good practice to ensure consistency across tests.testground/benchmark/benchmark/peer.py (2)
Line range hint
64-87
: LGTM! The changes improve account management efficiency.The updates to the
init_node
function enhance the efficiency and adaptability of account management during node initialization:
- The
global_seq
parameter ensures unique accounts are generated for each participant.- The
gen_account
function generates Ethereum-compatible addresses for additional accounts.However, please note that the unsafe import method used for the validator account provides greater flexibility but may have security implications. Ensure that the risks associated with this approach are carefully considered and mitigated.
120-128
: Excellent optimization! The use of a temporary file for bulk account addition significantly improves efficiency.The modification to the
gen_genesis
function to utilize a temporary file for bulk account addition is a great optimization. This change streamlines the process of adding multiple genesis accounts in a single command, reducing the number of CLI calls required during the genesis block creation.This enhancement significantly improves the efficiency of the genesis block generation process.
Solution:
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
PR Checklist:
make
)make test
)go fmt
)golangci-lint run
)go list -json -m all | nancy sleuth
)Thank you for your code, it's appreciated! :)
Summary by CodeRabbit
New Features
Balance
class for improved representation of account balances.Bug Fixes
Documentation