Updates to FeeSettings ledger object#758
Conversation
- Re-organize the _calculate_fee_per_transaction_type unit test into appropriate file - Expose a sync-variation of _calculate_fee_per_transaction_type method. This is needed for integration tests concerning SyncClient instances
…the term BaseFee to reflect non-availability of latest data
Update integration tests to validate non-historical transaction cost
WalkthroughThe pull request introduces several modifications primarily focused on the configuration of the rippled server, updates to the changelog, and enhancements to the transaction fee calculation logic. Key changes include the addition of a new Changes
Possibly related PRs
Suggested reviewers
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
xrpl/transaction/main.py (1)
136-159: Consider alternative toasyncio.run()for better flexibilityThe use of
asyncio.run()to execute the asynchronous function synchronously is straightforward, but it might cause issues if this function is called from an async context. Consider providing both sync and async versions of this function for more flexibility.Here's a suggestion to improve flexibility:
import asyncio from typing import Optional, Union # ... (other imports) async def _calculate_fee_per_transaction_type_async( transaction: Transaction, client: SyncClient, signers_count: Optional[int] = None, ) -> str: return await main._calculate_fee_per_transaction_type(transaction, client, signers_count) def _calculate_fee_per_transaction_type( transaction: Transaction, client: SyncClient, signers_count: Optional[int] = None, ) -> str: return asyncio.run(_calculate_fee_per_transaction_type_async(transaction, client, signers_count))This approach provides both synchronous and asynchronous versions of the function, allowing for more flexible usage in different contexts.
Consider using a more specific return type
The current return type hint is
str, which is quite generic for a fee value. Consider using a more specific type, such asintor a custom type that represents drops, to make the function's output clearer.Here's a suggestion for a more specific return type:
from typing import NewType Drops = NewType('Drops', int) def _calculate_fee_per_transaction_type( transaction: Transaction, client: SyncClient, signers_count: Optional[int] = None, ) -> Drops: # ... (implementation)Overall implementation looks good
The function is well-implemented and aligns with the PR objectives. It correctly uses the
Clientparameter as required, and the docstring provides clear information about the function's purpose and parameters.xrpl/models/base_model.py (1)
180-182: Improved type handling forAnyparametersThis change enhances the type checking capabilities of the
_from_dict_single_parammethod. By explicitly checking forparam_type is Any, it allows for more flexible handling of parameters without compromising type safety. This modification eliminates the need for a type ignore comment, making the code more robust and easier to maintain.Consider adding a brief comment explaining the purpose of this condition, such as:
if param_type is Any: # Allow any value for parameters typed as Any return param_valueThis would help future maintainers understand the intention behind this specific check.
tests/integration/sugar/test_transaction.py (2)
196-196: LGTM: Improved clarity in fee calculation comment.The updated comment provides a clearer explanation of the fee calculation formula. The variable name change from
base_feetonet_feeis also an improvement.Consider adding a brief comment explaining why the fee is calculated this way, if there's a specific reason for this formula.
440-467: New test added for fee calculation with issued currency.The new test
test_basic_calculate_fee_per_transaction_type_offlineis a valuable addition, covering fee calculation for a payment transaction with issued currency. However, there are a few points to consider:
The test is using a private function
_calculate_fee_per_transaction_type. Consider if this should be a public API or if the test should be moved to a more appropriate location.The expected fee is hardcoded as "200". This might make the test brittle if fee calculations change in the future.
Consider the following improvements:
- If
_calculate_fee_per_transaction_typeis meant to be a public API, rename it without the leading underscore. Otherwise, consider moving this test to be closer to the implementation it's testing.- Instead of hardcoding the expected fee, calculate it based on the current fee settings or make it clear why "200" is the expected value (e.g., is it a minimum fee for issued currency transactions?).
- Add more assertions to verify other properties of the created
Paymentobject to ensure it's constructed correctly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- .ci-config/rippled.cfg (1 hunks)
- tests/integration/sugar/test_transaction.py (4 hunks)
- tests/unit/core/addresscodec/test_main.py (0 hunks)
- xrpl/asyncio/transaction/init.py (2 hunks)
- xrpl/asyncio/transaction/main.py (5 hunks)
- xrpl/models/base_model.py (1 hunks)
- xrpl/transaction/init.py (2 hunks)
- xrpl/transaction/main.py (2 hunks)
💤 Files with no reviewable changes (1)
- tests/unit/core/addresscodec/test_main.py
🧰 Additional context used
🔇 Additional comments (14)
xrpl/asyncio/transaction/__init__.py (2)
Line range hint
2-9: LGTM: Import statement is correctly implemented.The new function
_calculate_fee_per_transaction_typeis properly imported from thexrpl.asyncio.transaction.mainmodule, maintaining consistency with the existing import structure.
Line range hint
11-27: Reconsider exposing_calculate_fee_per_transaction_typein__all__.The function
_calculate_fee_per_transaction_typeis being added to the__all__list, which exposes it as part of the public API. However, the function name starts with an underscore, which is a Python convention for indicating internal use. This seems inconsistent.Consider one of the following actions:
- If the function is intended for public use, rename it without the leading underscore.
- If the function is intended for internal use, remove it from the
__all__list.Please clarify the intended usage of this function and adjust accordingly.
To help verify the intended usage of this function, let's check its implementation and any documentation:
xrpl/transaction/__init__.py (1)
Line range hint
1-28: Clarify the purpose of these changes in relation to PR objectives.The changes in this file introduce a private function
_calculate_fee_per_transaction_typeto the module's imports and__all__list. While this might be related to updating fee calculations, it's not immediately clear how these changes contribute to the PR's stated objective of removing references to "10 drops" as the base transaction cost.Could you provide more context on how these changes align with the PR objectives? Additionally, consider running the following script to check for any remaining references to "10 drops" in the codebase:
✅ Verification successful
Verification Successful: No remaining references to "10 drops" found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for "10 drops" references in Python files # Test: Search for "10 drops" in Python files rg --type python "10 drops"Length of output: 60
Script:
#!/bin/bash # Description: Search for "10 drops" references in Python files # Test: Search for "10 drops" in Python files rg --type py "10 drops"Length of output: 25
.ci-config/rippled.cfg (1)
185-190: Approve addition of [voting] section with clarification neededThe addition of the [voting] section is a good implementation for simulating various FeeSettings scenarios in standalone mode. This aligns with the PR objectives of updating FeeSettings.
However, there are a few points that need clarification:
- The reference_fee is set to 200 drops, which differs from the "10 drops" mentioned in the PR objectives. Is this intentional?
- Can you confirm that the account_reserve (20 XRP) and owner_reserve (5 XRP) values are correct for your use case?
To verify the consistency of these values across the codebase, please run the following script:
This will help ensure that these new values are consistently applied throughout the codebase.
✅ Verification successful
Please manually verify the consistency of
reference_fee,account_reserve, andowner_reservevalues across the codebase.While automated scripts did not identify any conflicting hardcoded
feeorreservevalues, it's essential to ensure that the newly added[voting]section aligns correctly with all parts of the application.
Approve addition of [voting] section with clarification on
reference_feeThe addition of the [voting] section correctly implements the simulation of various FeeSettings scenarios in standalone mode. Automated checks confirm that there are no conflicting hardcoded
feeorreservevalues in the codebase.However, the
reference_feeis set to 200 drops, which differs from the "10 drops" mentioned in the PR objectives. Please confirm if this change is intentional.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any hardcoded fee or reserve values in the codebase # Search for hardcoded fee values echo "Searching for hardcoded fee values:" rg --type python -i '(fee|drop).*?[0-9]+' -g '!*.cfg' # Search for hardcoded reserve values echo "Searching for hardcoded reserve values:" rg --type python -i '(reserve).*?[0-9]+' -g '!*.cfg'Length of output: 342
xrpl/transaction/main.py (2)
1-5: File structure and imports look goodThe overall file structure is maintained, and the necessary imports have been added to support the new functionality. The placement of the new function at the end of the file is appropriate and follows the existing code organization.
Line range hint
1-159: Summary: Changes align well with PR objectivesThe modifications to
xrpl/transaction/main.pysuccessfully address the PR objectives:
- The new
_calculate_fee_per_transaction_typefunction uses theClientparameter to accurately retrieve current fee values.- The implementation maintains backward compatibility by not altering existing functions.
- The changes support the removal of "10 drops" as the base transaction cost reference.
These updates contribute to a more dynamic and accurate fee calculation system for the XRPL client library.
xrpl/models/base_model.py (1)
180-182: Overall impact assessmentThis change improves type handling without altering the core functionality of the
BaseModelclass. It maintains backwards compatibility and doesn't introduce any performance overhead. However, to ensure robustness:
- Verify that existing tests pass with this change.
- Consider adding new test cases that specifically cover the handling of
Anytype parameters in the_from_dict_single_parammethod.You can use the following script to run the existing tests and check for any failures:
If you need assistance in creating additional test cases for
Anytype parameters, please let me know.tests/integration/sugar/test_transaction.py (2)
18-21: LGTM: New imports added for additional functionality.The new imports for
_calculate_fee_per_transaction_typeandIssuedCurrencyAmountare correctly added to support the new test case introduced later in the file.Also applies to: 25-25
219-222: LGTM: Improved clarity and simplified assertion in payment fee test.The updated comment provides a clearer explanation of what the expected fee represents. The assertion has been simplified by directly comparing
payment_autofilled.feewithexpected_fee, which is more straightforward and removes unnecessary string conversion.xrpl/asyncio/transaction/main.py (5)
400-400: Docstring correctly reflects updated fee-checking logicThe docstring for
_check_feenow accurately describes that the function checks if the transaction fee is higher than the expected transaction type fee.
459-459: Validate the fee calculation forEscrowFinishwith fulfillmentThe calculation for
base_feein theEscrowFinishtransaction with a fulfillment seems correct. It uses the formula:base_fee = math.ceil(net_fee * (33 + (len(fulfillment_bytes) / 16)))Ensure that
fulfillment_bytesis correctly encoded and that the division by16accurately reflects the fee calculation as per the XRPL documentation.
467-469: Confirm the fee assignment forAccountDeleteandAMM_CREATEtransactionsSetting
base_feeto the owner's reserve fee forAccountDeleteandAMM_CREATEtransactions is appropriate according to XRPL fee guidelines. Ensure that_fetch_owner_reserve_fee(client)correctly retrieves the necessary fee from the ledger.
426-429: Ensure all calls to_calculate_fee_per_transaction_typepass the requiredclientparameterThe
clientparameter in_calculate_fee_per_transaction_typehas been changed from optional to required. Please verify that all calls to this function have been updated to include theclientargument to avoid potential issues.Run the following script to identify any calls to
_calculate_fee_per_transaction_typethat may be missing theclientparameter:#!/bin/bash # Description: Find calls to `_calculate_fee_per_transaction_type` without the `client` parameter # Search for `_calculate_fee_per_transaction_type` calls in Python files rg --type py '_calculate_fee_per_transaction_type\(' -A 2
396-399: Ensure all calls to_check_feepass the requiredclientparameterThe
clientparameter in_check_feehas been changed from optional to required. Please verify that all calls to this function have been updated to include theclientargument to prevent runtime errors.Run the following script to identify any calls to
_check_feethat may be missing theclientparameter:✅ Verification successful
All
_check_feecalls correctly include theclientparameter.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find calls to `_check_fee` without the `client` parameter # Search for `_check_fee` calls in Python files rg --type py '_check_fee\(' -A 2Length of output: 1279
Script:
#!/bin/bash # Description: Find calls to `_check_fee` without the `client` parameter # Search for `_check_fee` calls in Python files rg --type py '_check_fee\(' -A 2Length of output: 888
| transaction_json_to_binary_codec_form, | ||
| ) | ||
| from xrpl.transaction.main import ( | ||
| _calculate_fee_per_transaction_type, |
There was a problem hiding this comment.
Reconsider importing a private function.
The import of _calculate_fee_per_transaction_type raises a concern. Importing private functions (indicated by the leading underscore) from other modules is generally discouraged as it can lead to tight coupling and make the code more difficult to maintain.
Consider either:
- Making the function public if it's intended for use outside its original module, or
- Refactoring the code to avoid the need for importing this private function.
There was a problem hiding this comment.
This import is necessary for integration testing purposes. This method is used for internal purposes only.
There was a problem hiding this comment.
@ckeshava, understood. Thank you for the clarification.
（・_・）
✏️ Learnings added
Learnt from: ckeshava
PR: XRPLF/xrpl-py#758
File: xrpl/transaction/__init__.py:8-8
Timestamp: 2024-10-17T17:45:46.326Z
Learning: In the xrpl-py project, importing private functions (indicated by a leading underscore) for integration testing purposes is acceptable when the methods are used internally.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
In Python this isn't necessary and honestly is still bad practice. THe better route is to fix the integration tests to support this, which was done in #793.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@mvadari Hmm, so you'd recommend exporting a non-private method? One without a leading _ character?
There was a problem hiding this comment.
No, it's not useful for external people, so it shouldn't be exported or have a sync version. See what I did in the PR linked above (I can split it out into a separate PR if you'd prefer).
| "transaction_json_to_binary_codec_form", | ||
| "multisign", | ||
| "XRPLReliableSubmissionException", | ||
| "_calculate_fee_per_transaction_type", |
There was a problem hiding this comment.
Remove private function from all.
Adding _calculate_fee_per_transaction_type to the __all__ list is problematic. The __all__ list is meant to define the public API of a module, but this function is clearly intended to be private (as indicated by the leading underscore).
Remove this line from the __all__ list to maintain proper encapsulation and avoid exposing internal implementation details.
- "_calculate_fee_per_transaction_type",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "_calculate_fee_per_transaction_type", |
| # BaseFee × (1 + Number of Signatures Provided) | ||
| if signers_count is not None and signers_count > 0: | ||
| base_fee += net_fee * (1 + signers_count) | ||
| # Round Up base_fee and return it as a String |
There was a problem hiding this comment.
Review the multisign fee calculation logic
The fee calculation for multisignature transactions adds to base_fee as follows:
if signers_count is not None and signers_count > 0:
base_fee += net_fee * (1 + signers_count)Consider verifying whether the formula should multiply the base_fee instead of adding, based on the XRPL fee formula for multisigned transactions (BaseFee × (1 + Number of Signatures Provided)).
Apply this diff to correct the fee calculation:
- base_fee += net_fee * (1 + signers_count)
+ base_fee = base_fee * (1 + signers_count)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
CHANGELOG.md (1)
13-14: Approve changes with minor wording suggestion.The update correctly addresses the PR objective by removing the hard-coded reference to 10 drops as the base transaction cost. It also aligns with the goal of using current fee values by retrieving FeeSettings from the latest validated ledger.
Consider rephrasing for clarity:
- Grab the FeeSettings values from the latest validated ledger. Remove hard-coded reference to 10 drops as the reference transaction cost. + Retrieve FeeSettings values from the latest validated ledger. Remove hard-coded reference to 10 drops as the base transaction cost.
achowdhry-ripple
left a comment
achowdhry-ripple
left a comment
There was a problem hiding this comment.
LGTM - curious why we no longer want a default fee but otherwise looks good!
|
|
||
| # If FeeSettings ledger object is updated, the below assert needs to be changed | ||
| # correspondingly | ||
| self.assertEqual(fee, "200") |
There was a problem hiding this comment.
nit: could we use the reference_fee variable from the config file here so that only that file needs to be updated in the future?
There was a problem hiding this comment.
completed in 16099ec
I tried to use external libraries like toml and configparser. Thy could not parse the .ci-config/rippled.cfg file. Let me know if I can accomplish this without manually reading the file contents.
| net_fee = int(await get_fee(client)) # Usually 0.00001 XRP (10 drops) | ||
|
|
||
| net_fee = int( | ||
| await get_fee(client) |
There was a problem hiding this comment.
thought we should default to 200 drops always? Is the new recommendation that we don't have a default fee anymore?
There was a problem hiding this comment.
No, we do not have a default value. I used 200 drops as an example value, it was chosen arbitrarily.
We will know the transaction fees once all the validators have reached a compromise. Here is more detail on the Fee Voting process: https://xrpl.org/docs/concepts/consensus-protocol/fee-voting#voting-process
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
.github/workflows/snippet_test.yml (1)
Line range hint
52-53: Consider improving snippet execution for better error reporting.The current method of running snippets in a loop is functional, but it could be enhanced for better error reporting and maintainability. Here's a suggestion to improve this step:
Consider replacing the current snippet execution with a dedicated Python script. This approach would allow for more detailed error reporting and easier maintenance. Here's an example of how you could modify this step:
- Create a new file
run_snippets.pyin your repository:import os import sys import subprocess def run_snippets(): snippets_dir = 'snippets' for filename in os.listdir(snippets_dir): if filename.endswith('.py'): print(f"Running {filename}") result = subprocess.run(['poetry', 'run', 'python', os.path.join(snippets_dir, filename)], capture_output=True, text=True) if result.returncode != 0: print(f"Error in {filename}:") print(result.stderr) return 1 return 0 if __name__ == '__main__': sys.exit(run_snippets())
- Update the workflow step to use this script:
- name: Run Snippets run: poetry run python run_snippets.pyThis approach provides more structured error output and allows for easier addition of logging or other features in the future.
.github/workflows/integration_test.yml (1)
Line range hint
1-78: Approve overall workflow structure with a suggestion for improvement.The workflow structure is well-organized and includes necessary steps for setting up the environment, running tests, and cleaning up. The use of caching for Poetry and Python dependencies is a good practice for improving workflow efficiency.
However, to further enhance the workflow, consider the following suggestion:
Add a step to validate the rippled Docker container's health before running the tests. This can help catch any issues with the rippled service early in the workflow. Add the following step after the "Run docker in background" step:
- name: Wait for rippled to be healthy run: | timeout 60s bash -c 'while [[ "$(docker inspect -f {{.State.Health.Status}} rippled-service)" != "healthy" ]]; do sleep 5; done' || exit 1This step will wait for up to 60 seconds for the rippled container to report a healthy status before proceeding with the tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- .github/workflows/integration_test.yml (1 hunks)
- .github/workflows/snippet_test.yml (1 hunks)
- .github/workflows/unit_test.yml (1 hunks)
🧰 Additional context used
🔇 Additional comments (6)
.github/workflows/snippet_test.yml (2)
Line range hint
1-53: Overall, the workflow looks well-structured and up-to-date.The update to Poetry version 1.8.3 is the main change in this file, and it appears to be a positive improvement. The workflow's structure, including the use of caching and matrix strategy for Python versions, is well-implemented.
The only suggestion for improvement is related to the snippet execution step, which could be enhanced for better error reporting and maintainability as mentioned in the previous comment.
10-10: Poetry version update looks good, but consider potential impacts.The update of
POETRY_VERSIONfrom 1.4.2 to 1.8.3 is a significant version jump. This change is likely beneficial as it brings in the latest features and bug fixes from Poetry.However, please consider the following:
- Ensure that all dependencies in your project are compatible with Poetry 1.8.3.
- Check if there are any breaking changes in Poetry between these versions that might affect your workflow.
- It might be worth updating the
pyproject.tomlfile (if not already done) to specify the new minimum Poetry version required for the project.To verify the compatibility and impact of this change, you can run the following script:
This script will help identify any potential issues related to the Poetry version update.
.github/workflows/integration_test.yml (1)
4-4: Approve Poetry version update with verification steps.The update of POETRY_VERSION from 1.4.2 to 1.8.3 is a good practice for keeping dependencies up-to-date. However, given the significant version jump, please ensure the following:
- Verify compatibility with all Python versions in the matrix (3.8 to 3.12).
- Check if there are any breaking changes in Poetry 1.8.3 that might affect this workflow.
- Review the project's
pyproject.tomlfile to ensure it's compatible with Poetry 1.8.3.To verify Poetry's compatibility, run the following script:
.github/workflows/unit_test.yml (3)
Line range hint
1-114: Overall, the workflow update looks good with some points to address.The update to Poetry version 1.8.3 is the only change in this workflow file. While this update is likely beneficial, please address the following points:
- Clarify the reason for updating Poetry in the context of this PR's objectives.
- Verify compatibility of the new Poetry version with the project's dependencies.
- Ensure that all Poetry commands used in the workflow are still compatible with version 1.8.3.
Once these points are addressed, the workflow update should be good to go.
10-10: Clarify the reason for updating Poetry version.While updating Poetry is generally a good practice, this change doesn't seem directly related to the PR objectives of removing "10 drops" references and updating fee-related functionality. Could you please clarify why this update was necessary in the context of this PR?
To help understand if this change is related to any new dependencies or testing requirements, please run the following script:
#!/bin/bash # Description: Check for any new dependencies or testing requirements that might necessitate the Poetry update # Test: Compare dependencies between the old and new Poetry versions echo "Checking for dependency changes..." # Fetch the old pyproject.toml from the main branch git show main:pyproject.toml > pyproject.toml.old # Compare dependencies echo "Old dependencies:" grep '^\[tool.poetry.dependencies\]' -A 100 pyproject.toml.old | grep -v '^\[' | grep -v '^$' echo "New dependencies:" grep '^\[tool.poetry.dependencies\]' -A 100 pyproject.toml | grep -v '^\[' | grep -v '^$' echo "Differences in dependencies:" diff <(grep '^\[tool.poetry.dependencies\]' -A 100 pyproject.toml.old | grep -v '^\[' | grep -v '^$') <(grep '^\[tool.poetry.dependencies\]' -A 100 pyproject.toml | grep -v '^\[' | grep -v '^$') # Check for changes in dev dependencies or test requirements echo "Checking for changes in dev dependencies or test requirements..." diff <(grep '^\[tool.poetry.dev-dependencies\]' -A 100 pyproject.toml.old | grep -v '^\[' | grep -v '^$') <(grep '^\[tool.poetry.dev-dependencies\]' -A 100 pyproject.toml | grep -v '^\[' | grep -v '^$')This script will help identify any new dependencies or testing requirements that might explain the need for the Poetry version update.
10-10: Poetry version update looks good, but let's verify compatibility.The Poetry version has been updated from 1.4.2 to 1.8.3, which is a significant version jump. This update is likely to bring improvements and bug fixes.
To ensure this change doesn't introduce any compatibility issues, please run the following verification script:
This script will verify that the new Poetry version is compatible with your project's configuration and dependencies.
|
|
||
| env: | ||
| POETRY_VERSION: 1.4.2 | ||
| POETRY_VERSION: 1.8.3 |
There was a problem hiding this comment.
💡 Codebase verification
Poetry is not installed in the workflow environment.
The workflow updates the Poetry version to 1.8.3 but does not include a step to install Poetry. This will cause any Poetry commands in the workflow to fail.
-
Solution: Add a step to install Poetry before running Poetry commands. For example:
- name: Install Poetry run: | curl -sSL https://install.python-poetry.org | python3 - export PATH="$HOME/.local/bin:$PATH"
🔗 Analysis chain
Verify Poetry command compatibility with the new version.
While the workflow has been updated to use Poetry 1.8.3, it's important to ensure that all Poetry commands used in the workflow are still compatible with this new version. Please review the Poetry changelog for any breaking changes or new features that might affect the workflow.
To help with this verification, please run the following script:
This script will help verify that the Poetry commands used in the workflow are still valid and highlight any new features or changes that might be relevant to the workflow.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check Poetry command compatibility and new features
# Test: Verify Poetry commands used in the workflow
echo "Checking Poetry command compatibility..."
poetry_commands=("install" "run")
for cmd in "${poetry_commands[@]}"; do
echo "Checking 'poetry $cmd' command..."
poetry help $cmd | grep -q "Usage:" && echo "'poetry $cmd' is available" || echo "Warning: 'poetry $cmd' might have changed or is not available"
done
# Check for new relevant features or changes
echo "Checking for new relevant features or changes in Poetry 1.8.3..."
poetry --version
poetry help | grep -E "new|changed"
Length of output: 1222
Script:
#!/bin/bash
# Description: Verify if Poetry is installed and available in PATH
echo "Verifying Poetry installation..."
# Check if Poetry is installed
if command -v poetry >/dev/null 2>&1; then
echo "Poetry is installed."
echo "Poetry version:"
poetry --version
else
echo "Poetry is not installed or not in PATH."
fi
# Display current PATH
echo "Current PATH:"
echo $PATH
# Attempt to locate Poetry executable
echo "Locating Poetry executable..."
which poetry || echo "Poetry executable not found."
Length of output: 574
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
tests/integration/sugar/test_transaction.py (1)
440-478: LGTM: New test method for fee calculation with issued currency.The new test method
test_basic_calculate_fee_per_transaction_type_offlineis a valuable addition to verify fee calculation for payment transactions involving issued currencies. It uses the_calculate_fee_per_transaction_typefunction and compares the result with an expected fee read from a configuration file.Consider updating the comment on lines 472-473 to be more precise:
- # This step assumes that no non-`voting` section in the config file - # uses the reference_fee key-value pair. + # This step assumes that 'reference_fee' is only used in the [voting] + # section of the config file.This change more accurately reflects the assumption being made about the config file structure.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- tests/integration/sugar/test_transaction.py (4 hunks)
🧰 Additional context used
🔇 Additional comments (3)
tests/integration/sugar/test_transaction.py (3)
18-21: LGTM: Import statements updated correctly.The new imports are necessary for the added functionality in the test file. The
_calculate_fee_per_transaction_typefunction is used in the new test method, andIssuedCurrencyAmountis used to create test data.Also applies to: 25-25
196-196: LGTM: Improved comment clarity.The updated comment provides a clearer explanation of the fee calculation formula for the EscrowFinish transaction.
219-220: LGTM: Enhanced comment clarity for fee calculation.The updated comments provide a more precise explanation of the expected fee calculation for the Payment transaction.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
tests/integration/transactions/test_xchain_claim.py (3)
22-22: LGTM: Test decorator updated to handle async fee retrieval.The
@test_async_and_syncdecorator is correctly updated to include["xrpl.ledger.get_fee"], allowing the test to handle asynchronous calls for fetching transaction fees. This aligns with the PR objective of using theClientparameter to retrieve current fee values.Consider adding a comment explaining why
["xrpl.ledger.get_fee"]is included in the decorator, to improve code readability:@test_async_and_sync(globals(), ["xrpl.ledger.get_fee"]) # Include get_fee for async fee retrieval
101-101: LGTM: Transaction fee retrieval implemented.The addition of
transaction_fee = int(await get_fee(client))correctly implements the retrieval of the current fee, aligning with the PR objectives. The use ofawaitis appropriate for asynchronous fee retrieval.Consider adding error handling to make the code more robust:
try: transaction_fee = int(await get_fee(client)) except Exception as e: self.fail(f"Failed to retrieve transaction fee: {e}")This will provide more informative error messages if fee retrieval fails.
Line range hint
1-107: Summary: Changes align with PR objectives, but verification needed.The modifications to this test file correctly implement the retrieval and incorporation of transaction fees into the balance calculations, aligning with the PR objectives. The changes include:
- Adding the necessary import for
get_fee.- Updating the test decorator to handle async fee retrieval.
- Implementing transaction fee retrieval.
- Updating the final balance assertion to account for the fee.
These changes should improve the accuracy of the test by reflecting real-world fee scenarios. However, given the mention in the PR objectives that this file was failing some tests, it's crucial to verify if these changes have resolved those issues.
Next steps:
- Run the verification script provided in the previous comment to check if the test passes with the new changes.
- If the test still fails, investigate the specific assertions that are failing and consider if further adjustments are needed to the balance calculations or other parts of the test setup.
- Ensure that the test environment accurately reflects the expected
FeeSettingsvalues mentioned in the PR objectives.Once these steps are completed, we can be more confident in the robustness of this test implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- tests/integration/transactions/test_xchain_claim.py (3 hunks)
🧰 Additional context used
🔇 Additional comments (2)
tests/integration/transactions/test_xchain_claim.py (2)
7-7: LGTM: Import statement forget_feeadded.The new import statement for
get_feefromxrpl.asyncio.ledgeris correctly added and will be used for fee calculations later in the test.
104-107: LGTM: Final balance calculation updated to include transaction fee.The modification to subtract
transaction_feefrom the final balance calculation is correct and aligns with the PR objective of accurately handling fees in tests.However, the PR objectives mention that this file currently fails to pass all tests with the new
FeeSettingsvalues. Let's verify if this is still an issue:If the test is still failing, we may need to adjust the balance calculation further or investigate other factors affecting the test outcome.
|
|
||
| env: | ||
| POETRY_VERSION: 1.4.2 | ||
| POETRY_VERSION: 1.8.3 |
There was a problem hiding this comment.
Why do we need version bump here?
There was a problem hiding this comment.
poetry released version 1.8.3 6 months ago. I believe it is stable enough for our adoption: https://github.com/python-poetry/poetry/releases
Initially, I assumed the older version was causing a lint error, but the poetry version is not related to the linter.
I can revert back the poetry-related changes, if required.
There was a problem hiding this comment.
No issue at all. Just curious since poetry.lock already use 1.8.3 but I guess it doesn't update automatically in all places.
https://github.com/XRPLF/xrpl-py/blob/main/poetry.lock#L1
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
CHANGELOG.md (1)
16-17: Enhance the changelog entry for completeness.While the current entry correctly documents the removal of hard-coded fee settings, consider adding more details to help users understand the full scope of changes:
### Fixed -Grab the FeeSettings values from the latest validated ledger. Remove hard-coded reference to 10 drops as the reference transaction cost. +- Grab the FeeSettings values from the latest validated ledger by requiring Client parameter in check_fee method +- Remove hard-coded reference to 10 drops as the reference transaction cost +- Reorganize tests to support dynamic fee calculationThis enhanced entry better reflects:
- The mandatory Client parameter requirement
- The test reorganization
- Each change as a separate bullet point for clarity
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- CHANGELOG.md (1 hunks)
🔇 Additional comments (1)
CHANGELOG.md (1)
Line range hint
1-24: LGTM! The changelog structure follows best practices.The changelog properly follows the Keep a Changelog format and semantic versioning, with clear categorization of changes and proper placement of the unreleased section.
🧰 Tools
🪛 Markdownlint
13-13: Punctuation: ':'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
* - Mandate the `client` parameter inside check_fee method - Re-organize the _calculate_fee_per_transaction_type unit test into appropriate file - Expose a sync-variation of _calculate_fee_per_transaction_type method. This is needed for integration tests concerning SyncClient instances * remove hard-coded references to transaction BaseFee as 10 drops. Use the term BaseFee to reflect non-availability of latest data * Update rippled.cfg file to specify different FeeSettings options Update integration tests to validate non-historical transaction cost * Update poetry version to 1.8.3 inside Github Actions scripts * parse the expected fee from the config file * fix: Integration tests pertaining to XChainClaim pass * nit: rename the integration test to reflect updates
4 participants
High Level Overview of Change
This PR scrubs the xrpl-py client library of any references to "10 drops" as the Base transaction cost. The commits have been organized with the following intent:
Clientparameter inside thecheck_feemethod: Short of reading the latest validated ledger, I do not know of a way to grab the accurate Fees values.FeeSettingsledger objects.Context of Change
Type of Change
Did you update CHANGELOG.md?
Summary by CodeRabbit
Release Notes
New Features
[voting]section in server configuration with parameters for transaction fees and reserves.include_deletedparameter inledger_entryrequests.Bug Fixes
Breaking Changes