Deep freeze XLS-77d #803
Deep freeze XLS-77d #803
Conversation
|
Warning Rate limit exceeded@ckeshava has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 42 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThe pull request introduces the "DeepFreeze" feature across the project. The configuration file now includes a commented-out section and enabled "DeepFreeze" and "PermissionedDomains" flags. The changelog documents this new feature along with several additional updates and breaking changes, such as the removal of Python 3.7 support. Integration tests for offer creation and trust line management have been added to verify that deep freeze behavior is correctly enforced. Furthermore, new flags for deep freeze functionality are introduced in transaction models and unit tests. 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
Documentation and Community
|
![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
🧹 Nitpick comments (2)
tests/integration/transactions/test_offer_create.py (1)
63-66: Add assertion message for clarity.Consider adding a descriptive message to the assertion to clarify the expected behavior when the test fails.
self.assertEqual( offer.error.exception.args[0], "tecFROZEN", + "Offer creation should fail with tecFROZEN when trust line is deep frozen", )xrpl/models/transactions/trust_set.py (1)
50-55: Enhance documentation for deep freeze flag.The documentation should clarify the implications of deep freezing a trust line and reference the XLS-77d specification for more details.
""" Deep freeze the trust line. Allowed only if the trustline is already regularly frozen, or if tfSetFreeze is set in the same transaction. + See XLS-77d specification for more details on deep freeze functionality. + Note: Deep freezing a trust line prevents all transactions involving the frozen currency. """
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.ci-config/rippled.cfg(1 hunks)CHANGELOG.md(10 hunks)tests/integration/transactions/test_offer_create.py(2 hunks)tests/integration/transactions/test_trust_set.py(2 hunks)tests/unit/models/transactions/test_better_transaction_flags.py(1 hunks)xrpl/models/transactions/trust_set.py(2 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
CHANGELOG.md
22-22: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
120-120: Bare URL used
null
(MD034, no-bare-urls)
128-128: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
136-136: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
155-155: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
173-173: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
180-180: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
184-184: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
191-191: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
197-197: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
216-216: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
226-226: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
244-244: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
🪛 LanguageTool
CHANGELOG.md
[style] ~134-~134: Consider a shorter alternative to avoid wordiness.
Context: ...dress) - Added network_idto clients in order to use theClient` with networks beyond m...
(IN_ORDER_TO_PREMIUM)
[style] ~175-~175: This phrase is redundant. Consider writing “outcome”.
Context: ..., submit a transaction and wait for its final outcome - submit and `send_reliable_submissio...
(FINAL_END)
[uncategorized] ~220-~220: The preposition ‘to’ seems more likely in this position.
Context: ...o race condition - Add nft_offer type in AccountObjects - Handle errors better...
(AI_HYDRA_LEO_REPLACE_IN_TO)
[uncategorized] ~261-~261: Possible missing comma found.
Context: ...s. For each transaction type supporting flags there is a FlagInterface to set the f...
(AI_HYDRA_LEO_MISSING_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Snippet test (3.13)
- GitHub Check: Snippet test (3.12)
- GitHub Check: Snippet test (3.11)
- GitHub Check: Snippet test (3.10)
- GitHub Check: Snippet test (3.9)
- GitHub Check: Snippet test (3.8)
🔇 Additional comments (3)
.ci-config/rippled.cfg (1)
197-197: LGTM!The DeepFreeze feature is correctly added to the features list, following the established pattern.
tests/unit/models/transactions/test_better_transaction_flags.py (1)
279-280: LGTM! The deep freeze flags are correctly added to the test.The test follows the established pattern for flag testing and verifies that the new deep freeze flags are correctly handled by the TrustSet model.
CHANGELOG.md (1)
10-12: LGTM! The changelog entry is correctly formatted and placed.The entry follows the Keep a Changelog format and is placed in the correct section.
| response = await sign_and_reliable_submission_async( | ||
| TrustSet( | ||
| account=WALLET.address, | ||
| flags=TrustSetFlag.TF_SET_FREEZE & TrustSetFlag.TF_SET_DEEP_FREEZE, |
There was a problem hiding this comment.
Fix the flag combination logic.
The bitwise AND operator (&) is incorrect for combining flags. Use the bitwise OR operator (|) instead.
- flags=TrustSetFlag.TF_SET_FREEZE & TrustSetFlag.TF_SET_DEEP_FREEZE,
+ flags=TrustSetFlag.TF_SET_FREEZE | TrustSetFlag.TF_SET_DEEP_FREEZE,📝 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.
| flags=TrustSetFlag.TF_SET_FREEZE & TrustSetFlag.TF_SET_DEEP_FREEZE, | |
| flags=TrustSetFlag.TF_SET_FREEZE | TrustSetFlag.TF_SET_DEEP_FREEZE, |
There was a problem hiding this comment.
This is still unresolved.
There was a problem hiding this comment.
@khancode Yes, this needs to be fixed. The current code uses the bitwise AND operator (&) which will only keep bits that are set in both flags. This means if TF_SET_FREEZE is 0x00200000 and TF_SET_DEEP_FREEZE is 0x00400000, the result will be 0 since they have no bits in common.
The correct solution is to use the bitwise OR operator (|) to combine the flags:
flags=TrustSetFlag.TF_SET_FREEZE | TrustSetFlag.TF_SET_DEEP_FREEZE,This will properly set both flags by combining their bit patterns.
🧠 Learnings used
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl-py#803
File: tests/integration/transactions/test_trust_set.py:0-0
Timestamp: 2025-02-05T21:42:42.425Z
Learning: Failure cases for deep freeze functionality (e.g., setting deep freeze without regular freeze, clearing deep freeze while regular freeze is set) are handled by rippled server logic and don't need to be tested in the xrpl-py library.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/integration/transactions/test_offer_create.py(2 hunks)tests/integration/transactions/test_trust_set.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration/transactions/test_offer_create.py
🧰 Additional context used
📓 Learnings (1)
tests/integration/transactions/test_trust_set.py (1)
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl-py#803
File: tests/integration/transactions/test_trust_set.py:0-0
Timestamp: 2025-02-05T21:42:42.425Z
Learning: Failure cases for deep freeze functionality (e.g., setting deep freeze without regular freeze, clearing deep freeze while regular freeze is set) are handled by rippled server logic and don't need to be tested in the xrpl-py library.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Snippet test (3.13)
- GitHub Check: Snippet test (3.12)
- GitHub Check: Snippet test (3.11)
- GitHub Check: Snippet test (3.10)
- GitHub Check: Snippet test (3.9)
- GitHub Check: Snippet test (3.8)
🔇 Additional comments (2)
tests/integration/transactions/test_trust_set.py (2)
9-9: LGTM!The new imports are correctly placed and necessary for the deep freeze functionality test.
97-112: LGTM!The test setup is well-structured and correctly uses the bitwise OR operator for combining flags.
|
@achowdhry-ripple can you rebase to the recent |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
CHANGELOG.md (2)
22-25: BREAKING CHANGE Header Formatting in v4.0.0
The BREAKING CHANGE header on line 22 ends with a colon. According to markdown style guidelines, trailing punctuation in headings is discouraged. Consider removing the colon for consistency. For example:-### BREAKING CHANGE: +### BREAKING CHANGE🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
22-22: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
70-73: Removed Section Header Formatting in v2.6.0
The “Removed” section header on line 70 ends with a colon. To adhere to markdown guidelines (and the static analysis hints), it is recommended to remove the trailing punctuation. For example:-### Removed: +### RemovedThis small change will improve style consistency.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
70-70: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(10 hunks)tests/unit/models/transactions/test_better_transaction_flags.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/models/transactions/test_better_transaction_flags.py
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
CHANGELOG.md
22-22: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
121-121: Bare URL used
null
(MD034, no-bare-urls)
129-129: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
137-137: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
156-156: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
174-174: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
181-181: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
185-185: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
192-192: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
198-198: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
217-217: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
227-227: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
245-245: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
🪛 LanguageTool
CHANGELOG.md
[style] ~135-~135: Consider a shorter alternative to avoid wordiness.
Context: ...dress) - Added network_idto clients in order to use theClient` with networks beyond m...
(IN_ORDER_TO_PREMIUM)
[style] ~176-~176: This phrase is redundant. Consider writing “outcome”.
Context: ..., submit a transaction and wait for its final outcome - submit and `send_reliable_submissio...
(FINAL_END)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Snippet test (3.13)
- GitHub Check: Snippet test (3.12)
- GitHub Check: Snippet test (3.11)
- GitHub Check: Snippet test (3.10)
- GitHub Check: Snippet test (3.9)
- GitHub Check: Snippet test (3.8)
🔇 Additional comments (9)
CHANGELOG.md (9)
10-13: Deep Freeze Entry Added in Unreleased Section
The changelog now includes a new entry for “Support for Deep Freeze (XLS-77d)” under the Unreleased "Added" section. This clearly indicates the new feature and aligns with the PR objective.
17-20: New Additions for Version 4.0.0
The “Added” section for version 4.0.0 now documents support for the Multi-Purpose Tokens (MPT) amendment (XLS-33), the inclusion of theinclude_deletedparameter for ledger_entry requests, and XLS-70d (Credentials) support. These additions are clear and well formatted.
26-29: Fixed Section Update in Version 4.0.0
The updated Fixed section now documents the change to grab FeeSettings values from the latest validated ledger and remove the hard-coded fee reference. This entry is clear and informative.
32-35: BREAKING CHANGE Noted in Version 3.0.0
The BREAKING CHANGE entry in version 3.0.0, which specifies the switch to using rippled API v2 as the default in requests, is clearly documented. No changes are needed here, though consider adding any additional rationale if it would help users understand the impact.
36-40: Additional Features in Version 3.0.0
This section adds support for the DeliverMax field in Payment transactions and includes support for thefeatureRPC. The additions are clearly listed.
41-45: Fixes Detailed in Version 3.0.0 Fixed Section
The fixes for allowing empty strings in DIDSet transactions and updating the URL generation usingNetworkIDare effectively documented. Everything appears in order.
46-49: Deprecation of Parameters in Version 3.0.0 Removed Section
The removal of deprecated parameters (full,accounts, andtype) from the ledger request model is succinctly recorded. This entry is clear and requires no changes.
52-59: Comprehensive Additions in Version 2.6.0 Added Section
This section now covers several updates: support for the Price Oracles amendment (XLS-47), a new clio-only API fornfts_by_issuer, inclusion ofctidin the tx request, enhanced key handling infrom_xrpl, and support for the DynamicNFT amendment (XLS-46). The entries are comprehensive and consistent with the release notes.
60-69: Multiple Fixes Documented in Version 2.6.0 Fixed Section
The fixes address support forXChainModifyBridgeflag maps, validation improvements, IDE auto-completion support, handling of currency codes, and more. The detailed list clearly explains the corrections and improvements.🧰 Tools
🪛 LanguageTool
[style] ~62-~62: Consider using a different verb for a more formal wording.
Context: ...ort forXChainModifyBridgeflag maps (fixing an issue withNFTokenCreateOfferflag...(FIX_RESOLVE)
[typographical] ~66-~66: Usually, there’s no comma before “if”.
Context: ...of Wallet throws an "Invalid Seed" error, if the secret is not decode-able. - Rectif...(IF_NO_COMMA)
|
@ckeshava pulled in main but it still looks like it is failing. Do you know what the issue was / where the PR is for it |
|
Can you update the description to explain you're adding a deep freeze flag to an existing transaction? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
CHANGELOG.md (3)
26-28: Breaking Change: Python 3.7 Support Removal
The entry correctly marks the breaking change where Python 3.7 support is dropped in favor of Python 3.8. To fully conform with markdownlint MD026, consider removing the trailing colon from the heading (i.e. change “### BREAKING CHANGE:” to “### BREAKING CHANGE”).🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
26-26: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
133-140: Added: Enhanced Wallet Support and Generation Methods
The updates introduce support for regular key compatibility, various wallet generation methods, and the inclusion ofnetwork_idfor clients operating on multiple networks. Consider revising the trailing punctuation in the "Added:" heading to align with markdown lint standards.🧰 Tools
🪛 LanguageTool
[style] ~139-~139: Consider a shorter alternative to avoid wordiness.
Context: ...dress) - Addednetwork_idto clients in order to use theClient` with networks beyond m...(IN_ORDER_TO_PREMIUM)
🪛 markdownlint-cli2 (0.17.2)
133-133: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
1-369: Static Analysis & Markdown Style Reminders
Several markdownlint warnings have been raised (e.g. trailing punctuation in headings and use of bare URLs). Please review these style concerns and adjust them if they conflict with project guidelines. For example, removing trailing punctuation from headings (like “BREAKING CHANGE:” should be “BREAKING CHANGE”) may be beneficial.🧰 Tools
🪛 LanguageTool
[style] ~66-~66: Consider using a different verb for a more formal wording.
Context: ...ort forXChainModifyBridgeflag maps (fixing an issue withNFTokenCreateOfferflag...(FIX_RESOLVE)
[typographical] ~70-~70: Usually, there’s no comma before “if”.
Context: ...of Wallet throws an "Invalid Seed" error, if the secret is not decode-able. - Rectif...(IF_NO_COMMA)
[style] ~139-~139: Consider a shorter alternative to avoid wordiness.
Context: ...dress) - Addednetwork_idto clients in order to use theClient` with networks beyond m...(IN_ORDER_TO_PREMIUM)
[style] ~151-~151: Consider a shorter alternative to avoid wordiness.
Context: ... moving checks up to other functions) - In order to be internally consistent, all signing/s...(IN_ORDER_TO_PREMIUM)
[grammar] ~164-~164: This phrase is duplicated. You should probably use “now is” only once.
Context: ...pecifically -submit_transactionis nowsubmit-safe_sign_transactionis nowsign-safe_sign_and_submit_transactionis nowsign_and_submit- The param o...(PHRASE_REPETITION)
[typographical] ~170-~170: Consider adding a comma after ‘Specifically’ for more clarity.
Context: ...ust wrappers aroundClient.request(). Specifically this includes: -get_account_info...(RB_LY_COMMA)
[style] ~180-~180: This phrase is redundant. Consider writing “outcome”.
Context: ..., submit a transaction and wait for its final outcome -submitand `send_reliable_submissio...(FINAL_END)
[uncategorized] ~266-~266: Possible missing comma found.
Context: ...s. For each transaction type supporting flags there is aFlagInterfaceto set the f...(AI_HYDRA_LEO_MISSING_COMMA)
[duplication] ~293-~293: Possible typo: you repeated a word.
Context: ...nt methods -TicketCreatetransaction model -GenericRequestmodel for unsupported request types - Methods...(ENGLISH_WORD_REPEAT_RULE)
[uncategorized] ~356-~356: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...umentation - Exposexrpl.utilsat the top level - Expose `xrpl.accounts.get_account_roo...(EN_COMPOUND_ADJECTIVE_INTERNAL)
🪛 markdownlint-cli2 (0.17.2)
26-26: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
74-74: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
125-125: Bare URL used
null(MD034, no-bare-urls)
133-133: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
141-141: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
154-154: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
160-160: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
178-178: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
185-185: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
189-189: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
196-196: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
202-202: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
213-213: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
221-221: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
231-231: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
241-241: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
249-249: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CHANGELOG.md(11 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
CHANGELOG.md
26-26: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
125-125: Bare URL used
null
(MD034, no-bare-urls)
133-133: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
141-141: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
160-160: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
178-178: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
185-185: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
189-189: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
196-196: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
202-202: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
221-221: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
231-231: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
249-249: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
🪛 LanguageTool
CHANGELOG.md
[style] ~139-~139: Consider a shorter alternative to avoid wordiness.
Context: ...dress) - Added network_idto clients in order to use theClient` with networks beyond m...
(IN_ORDER_TO_PREMIUM)
[style] ~180-~180: This phrase is redundant. Consider writing “outcome”.
Context: ..., submit a transaction and wait for its final outcome - submit and `send_reliable_submissio...
(FINAL_END)
[uncategorized] ~266-~266: Possible missing comma found.
Context: ...s. For each transaction type supporting flags there is a FlagInterface to set the f...
(AI_HYDRA_LEO_MISSING_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: Snippet test (3.13)
- GitHub Check: Integration test (3.13)
- GitHub Check: Integration test (3.12)
- GitHub Check: Snippet test (3.12)
- GitHub Check: Unit test (3.10)
- GitHub Check: Integration test (3.11)
- GitHub Check: Snippet test (3.11)
- GitHub Check: Integration test (3.10)
- GitHub Check: Snippet test (3.10)
- GitHub Check: Integration test (3.9)
- GitHub Check: Snippet test (3.9)
- GitHub Check: Integration test (3.8)
- GitHub Check: Snippet test (3.8)
- GitHub Check: Lint and type-check
🔇 Additional comments (44)
CHANGELOG.md (44)
11-12: New Deep Freeze Feature Addition
The addition of Deep Freeze support (XLS-77d) in the Unreleased section is clearly documented and follows the established changelog format. Ensure that the implementation fully conforms to the XLS-77d specification.
21-25: Addition for MPT Amendment and Related Features
The changelog for version 4.0.0 now includes support for the Multi-Purpose Tokens (MPT) amendment, the inclusion of aninclude_deletedparameter in ledger_entry requests, and XLS-70d support. These entries are concise and consistent with the project's standards.
30-32: Fixed: FeeSettings Update
Updating the FeeSettings to retrieve values from the latest validated ledger—and removing the hard-coded fee reference—improves accuracy and maintainability.
36-38: Breaking Change: rippled API v2 Default
Designating the rippled API v2 as the default is a significant update. The breaking change is clearly flagged.
40-44: Addition: DeliverMax Field Support
The changelog now indicates support for the DeliverMax field in Payment transactions and the inclusion of thefeatureRPC. This enhancement is clearly and succinctly described.
45-49: Fixed: DIDSet Transaction Handling
Allowing empty strings for removing fields in the DIDSet transaction—and leveragingNetworkIDfor URL clarity—improves the overall robustness of the requests.
50-53: Removed Deprecated Ledger Request Parameters
Removing the deprecatedfull,accounts, andtypeparameters streamlines the ledger request model by eliminating outdated configuration options.
56-63: Added: Price Oracles Amendment and Additional Fields
The update adds support for the Price Oracles amendment (XLS-47), introduces thenfts_by_issuerAPI, includes thectidfield, and strengthens the key input handling infrom_xrpl. This entry is comprehensive and clear.
64-73: Fixed: XChainModifyBridge and IDE Autocompletion Enhancements
The fixes include proper handling of XChainModifyBridge flag maps, enhanced validation (e.g. updating flag names), and improvements for IDE auto-completion of model constructors.🧰 Tools
🪛 LanguageTool
[style] ~66-~66: Consider using a different verb for a more formal wording.
Context: ...ort forXChainModifyBridgeflag maps (fixing an issue withNFTokenCreateOfferflag...(FIX_RESOLVE)
[typographical] ~70-~70: Usually, there’s no comma before “if”.
Context: ...of Wallet throws an "Invalid Seed" error, if the secret is not decode-able. - Rectif...(IF_NO_COMMA)
74-77: Removed: Hooks Faucet Deprecation
Removing the Hooks faucet (now hosted on the Xahau testnet) is well documented here, keeping the changelog in sync with the current infrastructure.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
74-74: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
80-84: Added: DID Amendment and Server Definitions RPC
This entry documents the added support for the DID amendment (XLS-40) as well as for theserver_definitionsRPC, enhancing overall functionality.
85-89: Fixed: NFToken ID Export Issues
The improvements to exportingget_nftoken_idandparse_nftoken_id—and the clarification around error conditions when noNFTokenPageexists—enhance NFT processing clarity.
90-94: Changed: Deprecation of Old Faucet Supports
Marking the removal of sidechain-net1 and amm-devnet faucet supports helps guide users to the updated testing infrastructures.
96-101: Added: New SetFee Transaction Syntax and XChainBridge Support
The introduction of a new pseudo transaction syntax forSetFee(aligned with XRPFees) along with support for XLS-38d (XChainBridge) is clearly detailed and backward compatible.
102-107: Fixed: AMM Request Models and Error Handling
Enhancements regarding request models for AMM and improved error handling (including for Wallet parameter issues) strengthen transaction reliability.
108-113: Added: AMM Support Expansion
Adding support for Automated Market Maker (AMM) transactions as per XLS-30 is a significant enhancement that is clearly described.
114-119: Added: Clawback Transaction
The newly introducedClawbacktransaction (per XLS-39) is a welcome addition that broadens the transaction types supported.
120-126: Fixed: classic_address Alias Adjustment
By replacing the alias forclassic_addresswith a separate property, the changelog entry addresses a mypy issue effectively. The reference link provides further clarity.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
125-125: Bare URL used
null(MD034, no-bare-urls)
127-132: Breaking Change: Wallet Signing Algorithm Update
Switching the default signing algorithm from secp256k1 to ed25519 is a critical change. Please ensure accompanying documentation and migration guidelines are provided.
141-150: Changed: Wallet Class Constructor Updates
Updating the Wallet class constructor parameters, making the wallet address readonly, and removing obsolete fields (likesequence) contribute to a more consistent API.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
141-141: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
154-159: Fixed: Multisign Sorting and Ledger Enhancements
The improvements—such as sorting account IDs in multisign and addingledger_hashandnft_pageto requests—enhance the reliability of transaction processing.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
154-154: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
160-175: Removed: Deprecated Request Functions and Aliases
Eliminating legacy functions likesend_reliable_submissionand outdated request wrappers simplifies the API and reduces maintenance overhead.🧰 Tools
🪛 LanguageTool
[grammar] ~164-~164: This phrase is duplicated. You should probably use “now is” only once.
Context: ...pecifically -submit_transactionis nowsubmit-safe_sign_transactionis nowsign-safe_sign_and_submit_transactionis nowsign_and_submit- The param o...(PHRASE_REPETITION)
[typographical] ~170-~170: Consider adding a comma after ‘Specifically’ for more clarity.
Context: ...ust wrappers aroundClient.request(). Specifically this includes: -get_account_info...(RB_LY_COMMA)
🪛 markdownlint-cli2 (0.17.2)
160-160: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
177-184: Added: submit_and_wait and Additional Transaction Parameters
The addition of thesubmit_and_waitfunction—with support for additional parameters such asfail_hard,user_agent, andusage_context—provides enhanced control over transaction submission.🧰 Tools
🪛 LanguageTool
[style] ~180-~180: This phrase is redundant. Consider writing “outcome”.
Context: ..., submit a transaction and wait for its final outcome -submitand `send_reliable_submissio...(FINAL_END)
🪛 markdownlint-cli2 (0.17.2)
178-178: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
185-188: Changed: Keypair Signing Input Flexibility
Extending keypairs.sign to accept hexadecimal strings as well as bytes enhances flexibility when handling key inputs.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
185-185: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
189-195: Fixed: Transaction Existence and Error Handling Enhancements
Refactoring improvements in checking account existence and better error handling in transaction submission (including fixes for crashes in SignerListSet) bolster system stability.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
189-189: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
196-199: Removed: Legacy Sidechain Design Components
The removal of obsolete RPCs and utilities related to the old sidechain design cleans up the codebase and prevents potential confusion.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
196-196: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
201-212: Added: Transaction Signing Aliases and Wallet Enhancements
The newly introduced alias functions for signing transactions, along with added wallet enhancements (such asLedgerEntryTypeand seed encoding improvements), improve the developer experience.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
202-202: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
213-220: Changed: Request Adjustments and Deprecations
Increasing the check_fee limit, modifying defaults in generate_faucet_wallet, and deprecating outdated API endpoints contribute to a cleaner and more modern interface.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
213-213: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
221-228: Fixed: NestedModel Typing and Transaction Wait Duration
The refinements in NestedModel typing and the adjustments to waiting durations in reliable submissions help ensure more predictable behavior and better type safety.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
221-221: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
231-240: Added: Expanded Signer List and Metadata Parsing Support
Support for an expanded signer list, along with improved parsing of account balances and order book metadata, significantly enriches functionality within the XRPL ecosystem.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
231-231: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
241-246: Fixed: Factory Classmethods and Sphinx Build Errors
Corrections in factory class method typing and Sphinx build fixes improve both developer experience and documentation quality.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
241-241: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
248-255: Added: Dynamic Fee Calculation and Client Error Handling
Introducing dynamic fee calculation and enhancing client URL error handling addresses important operational scenarios and improves overall robustness.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
249-249: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
256-261: Fixed: Transaction Not Found and GenericRequest Bugs
Addressing thetxnNotFounderror and correcting bugs in GenericRequest methods ensure smoother operation during transaction submission.
263-270: Added: Flag Setting, RPC Support, and Payment Helpers
The changelog now reflects support for setting flags using booleans, the addition of federator_info RPC support, and several helper methods—features that expand the capabilities of the SDK.🧰 Tools
🪛 LanguageTool
[uncategorized] ~266-~266: Possible missing comma found.
Context: ...s. For each transaction type supporting flags there is aFlagInterfaceto set the f...(AI_HYDRA_LEO_MISSING_COMMA)
271-277: Fixed: NFT Naming and Client Exports
Updates to NFT names for newer rippled releases, improvements in client exports, and the introduction of an optional owner field in NFTokenBurn enhance clarity and functionality.
278-284: Added: Custom Faucet Host Support
Allowing both synchronous and asynchronousgenerate_faucet_walletfunctions to support a custom faucet host is a useful enhancement for testing and deployment flexibility.
285-300: Added: Innovative NFT Proposal and XRPL Model Enhancements
Support for the XLS-20 NFT proposal, along with a host of new helper functions and model improvements, further enriches the XRPL library’s feature set.🧰 Tools
🪛 LanguageTool
[duplication] ~293-~293: Possible typo: you repeated a word.
Context: ...nt methods -TicketCreatetransaction model -GenericRequestmodel for unsupported request types - Methods...(ENGLISH_WORD_REPEAT_RULE)
301-309: Fixed: Response Handling and Ledger Sequence Adjustments
Improvements to response typing, default ledger version settings in next valid sequence calculations, and refined error handling in reliable submission contribute to a more robust API.
315-320: Added: Python 3.10 Support
The addition of Python 3.10 support modernizes the codebase and expands its compatibility with current Python releases.
321-326: Fixed: Websocket Client Documentation and NoRippleCheck Issues
Enhancements in documentation for websocket clients and fixes related to NoRippleCheck requests help improve overall usability and clarity.
327-333: Fixed: Currency Encoding and Installation Documentation
The changelog update addressing issues with non-standard currency encoding and clarifying installation instructions improves both functionality and user guidance.
334-351: Added: Comprehensive Transaction Aliases and Async Support
Introducing new aliases for transaction-related functions along with asynchronous support significantly enhances API usability and consistency.
352-362: Fixed: Documentation Typos and Readability Enhancements
Correcting typos, improving the README/documentation readability, and exposing key modules at the top level collectively contribute to a better developer experience.🧰 Tools
🪛 LanguageTool
[uncategorized] ~356-~356: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...umentation - Exposexrpl.utilsat the top level - Expose `xrpl.accounts.get_account_roo...(EN_COMPOUND_ADJECTIVE_INTERNAL)
363-369: Added: Initial Release Notifications
The initial release note inviting feedback completes the changelog with historical context and encourages community engagement.
|
@achowdhry-ripple It appears that the docker container was never executed in the previous run of the CI. Hence we get the The CI appears to be running correctly now. |
| self.assertTrue( | ||
| ( | ||
| account_objects_response.result.account_obj[0] | ||
| | TrustSetFlag.TF_SET_DEEP_FREEZE |
There was a problem hiding this comment.
@achowdhry-ripple I still don't see the bitwise AND operator here. Have you pushed all of your recent commits upstream ?
|
Also update the description to be explicit in how this feature is implemented: new flag is added to an existing transaction. |
|
Updated the bitwise, lmk if you see it now |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/integration/transactions/test_trust_set.py (2)
97-137: Add docstring to explain test purpose.The test implementation looks good and follows the correct practices for flag operations. Consider adding a docstring to explain the test's purpose and expectations.
@test_async_and_sync(globals()) async def test_deep_freeze_functionality(self, client): + """ + Test the Deep Freeze functionality of TrustSet transaction. + Verifies that: + 1. TrustSet with both freeze and deep freeze flags succeeds + 2. The trust line is correctly marked as frozen + 3. The deep freeze flag is correctly set in account objects + """
131-137: Simplify the deep freeze flag verification.The current flag verification can be simplified for better readability.
- self.assertTrue( - ( - account_objects_response.result.account_obj[0] - & TrustSetFlag.TF_SET_DEEP_FREEZE - ) - != 0 - ) + self.assertTrue( + account_objects_response.result.account_obj[0] + & TrustSetFlag.TF_SET_DEEP_FREEZE, + "Deep freeze flag not set in account objects", + )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/integration/transactions/test_trust_set.py(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
tests/integration/transactions/test_trust_set.py (2)
Learnt from: ckeshava
PR: XRPLF/xrpl-py#803
File: tests/integration/transactions/test_trust_set.py:130-136
Timestamp: 2025-02-06T18:41:24.232Z
Learning: In Python, use the bitwise AND operator (`&`) to check if a specific flag is set in a flags value, not the bitwise OR operator (`|`). For example, to check if `TF_SET_DEEP_FREEZE` flag is set: `flags & TrustSetFlag.TF_SET_DEEP_FREEZE`.
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl-py#803
File: tests/integration/transactions/test_trust_set.py:0-0
Timestamp: 2025-02-05T21:42:42.425Z
Learning: Failure cases for deep freeze functionality (e.g., setting deep freeze without regular freeze, clearing deep freeze while regular freeze is set) are handled by rippled server logic and don't need to be tested in the xrpl-py library.
🔇 Additional comments (1)
tests/integration/transactions/test_trust_set.py (1)
9-9: LGTM!The new imports are correctly placed and necessary for the test implementation.
|
|
||
| self.assertTrue( | ||
| ( | ||
| account_objects_response.result.account_obj[0] |
There was a problem hiding this comment.
(I had made this mistake in my PR too, I rectified it in a later commit)
You need to use the lsf* flag, not the tf* flag. This should fix the integration test error.
TF corresponds to transaction-flags whereas we are validating the state of the ledger object. It's appropriate to check the Ledger-Specific-Flag (lsf).
There was a problem hiding this comment.
good catch, made the change
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.md (1)
11-14: Consider enhancing the Deep Freeze changelog entry.The changelog entry correctly documents the new feature. However, consider these enhancements for consistency and clarity:
-- Support for `Deep Freeze` (XLS-77d) +- Support for `Deep Freeze` ([XLS-77d](https://github.com/XRPLF/XRPL-Standards/discussions/77))Additionally, consider adding a brief description of what Deep Freeze does, similar to how other features are documented in the changelog. For example:
+- Support for `Deep Freeze` ([XLS-77d](https://github.com/XRPLF/XRPL-Standards/discussions/77)) - Allows issuers to freeze individual trustlines by setting and clearing deep freeze flags
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.ci-config/rippled.cfg(1 hunks)CHANGELOG.md(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .ci-config/rippled.cfg
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
CHANGELOG.md
28-28: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
127-127: Bare URL used
null
(MD034, no-bare-urls)
135-135: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
143-143: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
162-162: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
180-180: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
187-187: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
191-191: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
198-198: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
204-204: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
223-223: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
233-233: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
251-251: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
🪛 LanguageTool
CHANGELOG.md
[style] ~141-~141: Consider a shorter alternative to avoid wordiness.
Context: ...dress) - Added network_idto clients in order to use theClient` with networks beyond m...
(IN_ORDER_TO_PREMIUM)
[style] ~182-~182: This phrase is redundant. Consider writing “outcome”.
Context: ..., submit a transaction and wait for its final outcome - submit and `send_reliable_submissio...
(FINAL_END)
[uncategorized] ~246-~246: Possible missing preposition found.
Context: ...ng for factory classmethods on models - Use properly encoded transactions in Sign...
(AI_HYDRA_LEO_MISSING_TO)
[uncategorized] ~268-~268: Possible missing comma found.
Context: ...s. For each transaction type supporting flags there is a FlagInterface to set the f...
(AI_HYDRA_LEO_MISSING_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Integration test (3.13)
- GitHub Check: Snippet test (3.13)
- GitHub Check: Snippet test (3.12)
- GitHub Check: Integration test (3.12)
- GitHub Check: Integration test (3.11)
- GitHub Check: Snippet test (3.11)
- GitHub Check: Integration test (3.10)
- GitHub Check: Snippet test (3.10)
- GitHub Check: Integration test (3.9)
- GitHub Check: Snippet test (3.9)
- GitHub Check: Snippet test (3.8)
- GitHub Check: Integration test (3.8)
🔇 Additional comments (1)
CHANGELOG.md (1)
1-371: LGTM! The changelog structure is well-maintained.The changelog follows the Keep a Changelog format and Semantic Versioning guidelines consistently. The sections are clearly organized, making it easy to track changes across versions.
🧰 Tools
🪛 LanguageTool
[style] ~68-~68: Consider using a different verb for a more formal wording.
Context: ...ort forXChainModifyBridgeflag maps (fixing an issue withNFTokenCreateOfferflag...(FIX_RESOLVE)
[typographical] ~72-~72: Usually, there’s no comma before “if”.
Context: ...of Wallet throws an "Invalid Seed" error, if the secret is not decode-able. - Rectif...(IF_NO_COMMA)
[style] ~141-~141: Consider a shorter alternative to avoid wordiness.
Context: ...dress) - Addednetwork_idto clients in order to use theClient` with networks beyond m...(IN_ORDER_TO_PREMIUM)
[style] ~153-~153: Consider a shorter alternative to avoid wordiness.
Context: ... moving checks up to other functions) - In order to be internally consistent, all signing/s...(IN_ORDER_TO_PREMIUM)
[grammar] ~166-~166: This phrase is duplicated. You should probably use “now is” only once.
Context: ...pecifically -submit_transactionis nowsubmit-safe_sign_transactionis nowsign-safe_sign_and_submit_transactionis nowsign_and_submit- The param o...(PHRASE_REPETITION)
[typographical] ~172-~172: Consider adding a comma after ‘Specifically’ for more clarity.
Context: ...ust wrappers aroundClient.request(). Specifically this includes: -get_account_info...(RB_LY_COMMA)
[style] ~182-~182: This phrase is redundant. Consider writing “outcome”.
Context: ..., submit a transaction and wait for its final outcome -submitand `send_reliable_submissio...(FINAL_END)
[uncategorized] ~246-~246: Possible missing preposition found.
Context: ...ng for factory classmethods on models - Use properly encoded transactions inSign...(AI_HYDRA_LEO_MISSING_TO)
[uncategorized] ~268-~268: Possible missing comma found.
Context: ...s. For each transaction type supporting flags there is aFlagInterfaceto set the f...(AI_HYDRA_LEO_MISSING_COMMA)
[duplication] ~295-~295: Possible typo: you repeated a word.
Context: ...nt methods -TicketCreatetransaction model -GenericRequestmodel for unsupported request types - Methods...(ENGLISH_WORD_REPEAT_RULE)
[uncategorized] ~358-~358: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...umentation - Exposexrpl.utilsat the top level - Expose `xrpl.accounts.get_account_roo...(EN_COMPOUND_ADJECTIVE_INTERNAL)
🪛 markdownlint-cli2 (0.17.2)
28-28: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
76-76: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
127-127: Bare URL used
null(MD034, no-bare-urls)
135-135: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
143-143: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
156-156: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
162-162: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
180-180: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
187-187: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
191-191: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
198-198: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
204-204: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
215-215: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
223-223: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
233-233: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
243-243: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
251-251: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
|
|
||
| self.assertTrue( | ||
| ( | ||
| account_objects_response.result.account_obj[0] |
There was a problem hiding this comment.
you need to access the Flags field from this object. Print this out to understand the internal structure of the account_objects response
|
|
||
| @test_async_and_sync(globals()) | ||
| async def test_deep_freeze_functionality(self, client): | ||
| issuer_wallet = Wallet.create() |
There was a problem hiding this comment.
you also need to fund the issuer_wallet into existence. Wallet.create() only generates the public-private key pairs for an account.
We need to fund an account with a minimum XRP value for it to be recognised by the XRPL.
There was a problem hiding this comment.
I also thought this could be the error. @achowdhry-ripple I think it's worth trying to fund the account to see if the error gets resolved.
There was a problem hiding this comment.
@achowdhry-ripple
Here is a concrete example. You'll need to mimic this code:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/integration/transactions/test_trust_set.py (2)
14-15: Add documentation for the ledger-specific flags.Consider adding docstrings to explain the purpose and usage of these ledger-specific flags.
+# Ledger-specific flags for deep freeze functionality +# LSF_LOW_DEEP_FREEZE: Flag indicating low-level deep freeze state LSF_LOW_DEEP_FREEZE = 0x02000000 +# LSF_HIGH_DEEP_FREEZE: Flag indicating high-level deep freeze state LSF_HIGH_DEEP_FREEZE = 0x04000000
143-149: Simplify the deep freeze flags check.The current check can be simplified to improve readability.
- self.assertTrue( - ( - account_objects_response.result.account_obj[0] - & (LSF_LOW_DEEP_FREEZE | LSF_HIGH_DEEP_FREEZE) - ) - != 0 - ) + flags = account_objects_response.result.account_obj[0].Flags + self.assertTrue( + flags & LSF_LOW_DEEP_FREEZE or flags & LSF_HIGH_DEEP_FREEZE, + "Neither deep freeze flag is set", + )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/integration/transactions/test_offer_create.py(2 hunks)tests/integration/transactions/test_trust_set.py(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
tests/integration/transactions/test_offer_create.py (2)
Learnt from: ckeshava
PR: XRPLF/xrpl-py#803
File: tests/integration/transactions/test_trust_set.py:130-136
Timestamp: 2025-02-06T18:41:24.232Z
Learning: In Python, use the bitwise AND operator (`&`) to check if a specific flag is set in a flags value, not the bitwise OR operator (`|`). For example, to check if `TF_SET_DEEP_FREEZE` flag is set: `flags & TrustSetFlag.TF_SET_DEEP_FREEZE`.
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl-py#803
File: tests/integration/transactions/test_trust_set.py:0-0
Timestamp: 2025-02-05T21:42:42.425Z
Learning: Failure cases for deep freeze functionality (e.g., setting deep freeze without regular freeze, clearing deep freeze while regular freeze is set) are handled by rippled server logic and don't need to be tested in the xrpl-py library.
tests/integration/transactions/test_trust_set.py (2)
Learnt from: ckeshava
PR: XRPLF/xrpl-py#803
File: tests/integration/transactions/test_trust_set.py:130-136
Timestamp: 2025-02-06T18:41:24.232Z
Learning: In Python, use the bitwise AND operator (`&`) to check if a specific flag is set in a flags value, not the bitwise OR operator (`|`). For example, to check if `TF_SET_DEEP_FREEZE` flag is set: `flags & TrustSetFlag.TF_SET_DEEP_FREEZE`.
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl-py#803
File: tests/integration/transactions/test_trust_set.py:0-0
Timestamp: 2025-02-05T21:42:42.425Z
Learning: Failure cases for deep freeze functionality (e.g., setting deep freeze without regular freeze, clearing deep freeze while regular freeze is set) are handled by rippled server logic and don't need to be tested in the xrpl-py library.
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: Integration test (3.13)
- GitHub Check: Unit test (3.12)
- GitHub Check: Snippet test (3.13)
- GitHub Check: Snippet test (3.12)
- GitHub Check: Integration test (3.12)
- GitHub Check: Snippet test (3.11)
- GitHub Check: Integration test (3.11)
- GitHub Check: Snippet test (3.10)
- GitHub Check: Integration test (3.10)
- GitHub Check: Integration test (3.9)
- GitHub Check: Unit test (3.8)
- GitHub Check: Snippet test (3.9)
- GitHub Check: Snippet test (3.8)
- GitHub Check: Integration test (3.8)
🔇 Additional comments (4)
tests/integration/transactions/test_offer_create.py (1)
31-69: LGTM! Well-structured test for deep freeze functionality.The test correctly verifies that offer creation fails with "tecFROZEN" when the trust line is deep frozen. Good job on:
- Using proper flag combination with the bitwise OR operator
- Including the
tesSUCCESScheck for trust line setup- Verifying the expected error
tests/integration/transactions/test_trust_set.py (3)
22-22: LGTM! Good improvements to the test.The changes enhance the test by:
- Funding the issuer wallet to ensure it exists in the ledger
- Adding explicit verification of transaction success
Also applies to: 37-37
42-42: LGTM! Good improvements to the test.The changes enhance the test by:
- Funding the issuer wallet to ensure it exists in the ledger
- Adding explicit verification of transaction success for each test case
Also applies to: 57-57, 73-73, 89-89
107-142: LGTM! Well-structured test for deep freeze functionality.The test thoroughly verifies the deep freeze functionality by:
- Setting up and funding the issuer wallet
- Using proper flag combination
- Verifying transaction success
- Checking freeze status in account lines
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/integration/transactions/test_offer_create.py (1)
31-32: Add docstring to explain test purpose.Consider adding a docstring to explain the test's purpose and flow. For example:
@test_async_and_sync(globals()) async def test_deep_freeze_trustline_fails(self, client): + """ + Test that offer creation fails with tecFROZEN when the trust line + is set with both TF_SET_FREEZE and TF_SET_DEEP_FREEZE flags. + + Steps: + 1. Create and fund issuer wallet + 2. Set up trust line with deep freeze flags + 3. Attempt to create offer (should fail) + """
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/integration/transactions/test_offer_create.py(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
tests/integration/transactions/test_offer_create.py (2)
Learnt from: ckeshava
PR: XRPLF/xrpl-py#803
File: tests/integration/transactions/test_trust_set.py:130-136
Timestamp: 2025-02-06T18:41:24.232Z
Learning: In Python, use the bitwise AND operator (`&`) to check if a specific flag is set in a flags value, not the bitwise OR operator (`|`). For example, to check if `TF_SET_DEEP_FREEZE` flag is set: `flags & TrustSetFlag.TF_SET_DEEP_FREEZE`.
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl-py#803
File: tests/integration/transactions/test_trust_set.py:0-0
Timestamp: 2025-02-05T21:42:42.425Z
Learning: Failure cases for deep freeze functionality (e.g., setting deep freeze without regular freeze, clearing deep freeze while regular freeze is set) are handled by rippled server logic and don't need to be tested in the xrpl-py library.
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Integration test (3.13)
- GitHub Check: Snippet test (3.13)
- GitHub Check: Snippet test (3.12)
- GitHub Check: Integration test (3.12)
- GitHub Check: Snippet test (3.11)
- GitHub Check: Integration test (3.11)
- GitHub Check: Integration test (3.10)
- GitHub Check: Snippet test (3.10)
- GitHub Check: Integration test (3.9)
- GitHub Check: Snippet test (3.9)
- GitHub Check: Integration test (3.8)
- GitHub Check: Snippet test (3.8)
🔇 Additional comments (2)
tests/integration/transactions/test_offer_create.py (2)
3-3: LGTM!The new imports are correctly added and necessary for the deep freeze functionality test.
Also applies to: 9-10
31-70: LGTM! Test implementation is thorough and correct.The test correctly verifies that offer creation fails with
tecFROZENwhen the trust line is deep frozen. Good job on checking both the trust line setup success and the expected offer failure.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/integration/transactions/test_delete_oracle.py (1)
56-59: Enhance assertion message for clarity.While the verification logic is correct, consider adding a more descriptive assertion message to clarify the expected state.
- self.assertEqual(len(account_objects_response.result["account_objects"]), 0) + self.assertEqual( + len(account_objects_response.result["account_objects"]), + 0, + "Expected no Oracle objects after deletion" + )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/integration/transactions/test_delete_oracle.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Integration test (3.13)
- GitHub Check: Integration test (3.12)
- GitHub Check: Integration test (3.11)
- GitHub Check: Integration test (3.10)
- GitHub Check: Integration test (3.9)
- GitHub Check: Integration test (3.8)
🔇 Additional comments (5)
tests/integration/transactions/test_delete_oracle.py (5)
8-8: LGTM! Consistent variable naming.The change from
WALLETtoDESTINATIONaligns with the standardization across the codebase.
24-41: LGTM! Well-structured test setup.The OracleSet transaction is well-documented and properly configured with test data. The account parameter update maintains consistency with the new naming convention.
42-44: LGTM! Proper transaction submission and validation.The transaction submission is correctly updated to use
DESTINATIONand includes appropriate response validation.
47-50: LGTM! Clean deletion setup.The OracleDelete transaction is properly configured with the updated account parameter.
51-53: LGTM! Proper transaction submission and validation.The deletion transaction submission is correctly updated and includes appropriate response validation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/integration/transactions/test_set_oracle.py (2)
26-56: Consider adding edge case tests.While the test coverage is good, consider adding test cases for:
- Zero or negative price values
- Invalid scale values
- Empty price data series
61-77: Enhance assertion clarity and completeness.The assertions could be more explicit and comprehensive:
- The length check could specify the exact expected count
- The hex string comparison is using
assertTrueincorrectlyApply this diff to improve the assertions:
- self.assertTrue(len(account_objects_response.result["account_objects"]) > 0) + self.assertEqual(len(account_objects_response.result["account_objects"]), 1) - self.assertTrue( + self.assertEqual( account_objects_response.result["account_objects"][0]["PriceDataSeries"][2][ "PriceData" ]["AssetPrice"], "ffffffffffffffff", - ) + )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/integration/transactions/test_set_oracle.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Snippet test (3.13)
- GitHub Check: Integration test (3.13)
- GitHub Check: Integration test (3.12)
- GitHub Check: Snippet test (3.12)
- GitHub Check: Integration test (3.11)
- GitHub Check: Snippet test (3.11)
- GitHub Check: Snippet test (3.10)
- GitHub Check: Integration test (3.10)
- GitHub Check: Integration test (3.9)
- GitHub Check: Snippet test (3.9)
- GitHub Check: Snippet test (3.8)
- GitHub Check: Integration test (3.8)
🔇 Additional comments (2)
tests/integration/transactions/test_set_oracle.py (2)
4-13: LGTM!The imports are correctly organized and include all necessary dependencies for the wallet creation and funding functionality.
24-25: Great improvement in test isolation!Creating a fresh wallet for each test run and funding it dynamically improves test isolation and reduces potential interference between test runs.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/integration/it_utils.py (1)
194-215: Consider refactoring to reduce code duplication.The OracleSet handling logic is duplicated between sync and async versions. Consider extracting the common logic into a shared helper function:
+def _modify_oracle_set_transaction(transaction: Transaction, ledger_close_time: int) -> Transaction: + if not isinstance(transaction, OracleSet): + return transaction + + transaction_json = transaction.to_dict() + transaction_json["last_update_time"] = ripple_time_to_posix(ledger_close_time) + return Transaction.from_dict(transaction_json) + def sign_and_reliable_submission(...): - modified_transaction = transaction - if isinstance(transaction, OracleSet): - transaction_json = transaction.to_dict() - last_validated_ledger = client.request(Ledger(ledger_index="validated")) - transaction_json["last_update_time"] = ripple_time_to_posix( - last_validated_ledger.result["ledger"]["close_time"] - ) - modified_transaction = Transaction.from_dict(transaction_json) + last_validated_ledger = client.request(Ledger(ledger_index="validated")) + modified_transaction = _modify_oracle_set_transaction( + transaction, + last_validated_ledger.result["ledger"]["close_time"] + ) async def sign_and_reliable_submission_async(...): - modified_transaction = transaction - if isinstance(transaction, OracleSet): - transaction_json = transaction.to_dict() - last_validated_ledger = await client.request(Ledger(ledger_index="validated")) - transaction_json["last_update_time"] = ripple_time_to_posix( - last_validated_ledger.result["ledger"]["close_time"] - ) - modified_transaction = Transaction.from_dict(transaction_json) + last_validated_ledger = await client.request(Ledger(ledger_index="validated")) + modified_transaction = _modify_oracle_set_transaction( + transaction, + last_validated_ledger.result["ledger"]["close_time"] + )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/integration/it_utils.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Integration test (3.13)
- GitHub Check: Snippet test (3.13)
- GitHub Check: Snippet test (3.12)
- GitHub Check: Integration test (3.12)
- GitHub Check: Integration test (3.11)
- GitHub Check: Snippet test (3.11)
- GitHub Check: Snippet test (3.10)
- GitHub Check: Integration test (3.10)
- GitHub Check: Integration test (3.9)
- GitHub Check: Snippet test (3.9)
- GitHub Check: Snippet test (3.8)
- GitHub Check: Integration test (3.8)
🔇 Additional comments (3)
tests/integration/it_utils.py (3)
21-21: LGTM! New imports are correctly organized.The new imports are properly placed with similar imports and are necessary for handling
OracleSettransactions.Also applies to: 24-24, 30-30
157-182: LGTM! Proper handling of OracleSet transactions.The implementation correctly handles the
tecINVALID_UPDATE_TIMEerror by using the last validated ledger's close time.
117-126: LGTM! Proper async implementation of fund_wallet.The async version correctly mirrors the sync version and follows the established patterns for async functions in the codebase.
| # Empirical observations display a difference of ~1200 seconds between the two. | ||
| # Note: The cause of this discrepancy is the LEDGER_ACCEPT_TIME parameter set | ||
| # to 0.1 seconds. Further investigation is required toward future updates to | ||
| # this parameter. |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Investigate the LEDGER_ACCEPT_TIME parameter.
The comment indicates a significant time discrepancy (~1200 seconds) between the local system clock and the standalone rippled node, potentially caused by the LEDGER_ACCEPT_TIME parameter being set to 0.1 seconds.
Let's investigate if this is a known issue:
🌐 Web query:
What is the recommended LEDGER_ACCEPT_TIME for rippled standalone mode?
💡 Result:
The recommended approach for advancing ledgers in rippled standalone mode involves manual control rather than a predefined LEDGER_ACCEPT_TIME parameter. Based on search results:
-
Manual ledger advancement is required using the
ledger_acceptcommand[3][5]. There's no automatic interval - you explicitly trigger ledger closure when ready. -
Common practice from operational examples shows 10-second intervals being used:
watch -n 10 /opt/ripple/bin/rippled --conf=/opt/ripple/etc/rippled.cfg ledger_acceptThis command advances ledgers every 10 seconds[1].
- Transaction ordering considerations:
- Transactions from the same address execute in
Sequencenumber order[5] - Dependent transactions from different addresses should have ledgers manually closed between submissions to ensure proper execution order[5]
There's no official "recommended" time interval - the frequency depends on your testing requirements. The 10-second example works for basic scenarios, but you might use shorter/longer intervals based on:
- Transaction complexity
- Debugging needs
- Performance testing goals
For automated testing, you could implement custom scripting around ledger_accept calls rather than relying on fixed intervals[1][6].
Citations:
- 1: https://www.d10.dev/blog/rippled-notes/
- 2: Augmented submit fields should be included in sign-and-submit mode (Version: 1.5.0-b6) rippled#3284
- 3: https://xrpl.org/docs/infrastructure/testing-and-auditing/load-a-saved-ledger-in-stand-alone-mode
- 4: https://xrpl.org/docs/references/http-websocket-apis/admin-api-methods/server-control-methods/ledger_accept
- 5: https://xrpl.org/docs/infrastructure/testing-and-auditing/advance-the-ledger-in-stand-alone-mode
- 6: https://dev.xagfans.com/use-stand-alone-mode.html
- 7: How to allow peer connections in stand-alone-like mode? rippled#2450
- 8: https://dev.xagfans.com/install-rippled.html
Action: Update Ledger Advancement Methodology
The current integration test documentation notes that LEDGER_ACCEPT_TIME is set to 0.1 seconds, which appears to be linked to an observed ~1200-second discrepancy. Based on recent findings for rippled standalone mode:
- Manual Ledger Advancement Preferred: The recommended practice is to advance ledgers manually—using commands like
ledger_accept—rather than relying on a fixed LEDGER_ACCEPT_TIME. - Interval Considerations: Operational examples often use a roughly 10-second automated interval (if automation is necessary), but no fixed parameter is officially endorsed.
- Next Steps: Revisit the testing configuration to verify if automated ledger advancement via LEDGER_ACCEPT_TIME is truly desired. If so, consider adjusting the interval to reflect practical operation (e.g., around 10 seconds) or switch to manual ledger advancement to align with current recommendations.
… from last_validated_ledger_close_time value to maintain clock synchronization use fresh wallets for OracleSet integ tests Use fresh wallets for OracleDelete integration tests
ckeshava
force-pushed
the
deep-freeze
branch
from
4ec16f1 to
d8c22b6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/integration/transactions/test_delete_oracle.py (1)
30-61: Add error handling for wallet funding.While the transaction execution is correct, consider adding error handling for the wallet funding operation to make the test more robust.
- await fund_wallet_async(oracle_owner_wallet) + try: + await fund_wallet_async(oracle_owner_wallet) + except Exception as e: + self.fail(f"Failed to fund test wallet: {str(e)}")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/integration/transactions/test_delete_oracle.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Integration test (3.13)
- GitHub Check: Snippet test (3.13)
- GitHub Check: Integration test (3.12)
- GitHub Check: Snippet test (3.12)
- GitHub Check: Integration test (3.11)
- GitHub Check: Snippet test (3.11)
- GitHub Check: Integration test (3.10)
- GitHub Check: Snippet test (3.10)
- GitHub Check: Integration test (3.9)
- GitHub Check: Snippet test (3.9)
- GitHub Check: Integration test (3.8)
- GitHub Check: Snippet test (3.8)
🔇 Additional comments (3)
tests/integration/transactions/test_delete_oracle.py (3)
4-13: LGTM! Clean import organization.The new imports for
fund_wallet_asyncandWalletare properly organized and necessary for the wallet management changes.
22-25: LGTM! Good test isolation practice.Creating a fresh wallet for each test run is a good practice as it:
- Prevents test conflicts by isolating wallet state
- Makes tests more reliable and repeatable
- Clearly documents the test's dependencies
66-71: LGTM! Thorough verification logic.The verification properly checks both the transaction success and the actual deletion of the Oracle object from the ledger.
|
Apologies for the force-push -- I've attempted to fix the OracleSet integration tests in the last commit. I don't completely understand why these tests flared up for this PR. I have tried to present a reasonable hypothesis in the relevant comments. I request that the last commit message be retained during the git-squash-merge operation for posterity. cc @khancode @achowdhry-ripple If you guys are happy with the PR, we can merge it. |
High Level Overview of Change
Add support for Deep freeze based off of proposed spec
https://github.com/XRPLF/XRPL-Standards/blob/dcb651403510237f26eeb854d550fa156d0e8b47/XLS-0077-deep-freeze/README.md
Context of Change
Type of Change
Did you update CHANGELOG.md?
Test Plan