Deep Freeze XLS-77d #2873
Deep Freeze XLS-77d #2873
Conversation
…t transaction integration test for OfferCreate txn (negative case) update to the unit test of TrustSet transaction
|
Warning Rate limit exceeded@ckeshava has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 25 minutes and 33 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)
WalkthroughThis pull request introduces support for the XLS-77d Deep-Freeze amendment in the XRPL ecosystem. The changes span multiple files across the xrpl.js library, adding new flags and configurations related to deep-freezing trust lines. Modifications include updating the configuration file, extending enum definitions with new flags, and adding test cases to verify the new deep-freeze functionality. The implementation allows for setting and clearing deep-freeze states on trust lines, which restricts sending and receiving of specified issued currencies. 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 (5)
packages/xrpl/src/models/ledger/RippleState.ts (1)
80-83: Add documentation comments for the new deep freeze flags.The new flags would benefit from detailed documentation comments explaining their purpose and behavior, similar to other flags in the enum.
// True, trust line to AMM. Used by client apps to identify payments via AMM. lsfAMMNode = 0x01000000, - // True, low side has set deep freeze flag + /** + * True if the low side has set the deep freeze flag. + * When set, this flag permanently freezes the trust line until explicitly cleared. + */ lsfLowDeepFreeze = 0x02000000, - // True, high side has set deep freeze flag + /** + * True if the high side has set the deep freeze flag. + * When set, this flag permanently freezes the trust line until explicitly cleared. + */ lsfHighDeepFreeze = 0x04000000,packages/xrpl/test/models/trustSet.test.ts (1)
25-29: Enhance test coverage for deep freeze flags.The test suite would benefit from additional test cases:
- Testing the
tfClearDeepFreezeflag- Negative test cases for invalid flag combinations
- Validation of flag values
// Example additional test cases: it('throws when setting both tfSetDeepFreeze and tfClearDeepFreeze', function () { trustSet.Flags = { tfSetDeepFreeze: true, tfClearDeepFreeze: true, } assert.throws( () => validateTrustSet(trustSet), ValidationError, 'TrustSet: cannot set and clear deep freeze simultaneously', ) }) it('verifies clearing of deep freeze', function () { trustSet.Flags = { tfClearDeepFreeze: true, } assert.doesNotThrow(() => validateTrustSet(trustSet)) })packages/xrpl/test/integration/transactions/offerCreate.test.ts (1)
62-108: Enhance test coverage for deep-frozen trust line scenarios.While the test verifies basic blocking behavior, consider adding test cases for:
- Attempting to modify an existing offer after deep-freezing
- Testing with different combinations of freeze flags (regular freeze vs deep freeze)
- Testing behavior when only one side of the trust line is deep-frozen
Also, consider extracting the trust line setup into a helper function to improve test readability and reusability:
async function setupDeepFrozenTrustLine( testContext: XrplIntegrationTestContext, counterparty: Wallet, currency: string = 'USD', value: string = '10', ): Promise<void> { const trust_set_tx: TrustSet = { TransactionType: 'TrustSet', Account: testContext.wallet.classicAddress, LimitAmount: { currency, issuer: counterparty.classicAddress, value, }, Flags: { tfSetFreeze: true, tfSetDeepFreeze: true, }, } await testTransaction(testContext.client, trust_set_tx, testContext.wallet) }packages/xrpl/src/models/transactions/trustSet.ts (2)
33-34: Fix typo in documentation commentThere's a typo in the comment: "recieving" should be "receiving".
- /** Deep-Freeze the trust line -- disallow sending and recieving the said IssuedCurrency */ + /** Deep-Freeze the trust line -- disallow sending and receiving the said IssuedCurrency */
96-99: Document interaction with regular freeze flagsThe documentation should clarify how deep-freeze flags interact with regular freeze flags (
tfSetFreeze/tfClearFreeze). This helps developers understand the precedence and combined effects.Add a note in the documentation:
/** Deep-Freeze the trust line -- disallow sending and receiving the said IssuedCurrency */ tfSetDeepFreeze?: boolean /** Clear a Deep-Frozen trust line */ tfClearDeepFreeze?: boolean + /** + * Note: Deep-freeze takes precedence over regular freeze. When a trust line is + * deep-frozen, the regular freeze state has no effect until the deep-freeze is cleared. + */
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.ci-config/rippled.cfg(1 hunks)packages/xrpl/HISTORY.md(1 hunks)packages/xrpl/src/models/ledger/RippleState.ts(1 hunks)packages/xrpl/src/models/transactions/trustSet.ts(2 hunks)packages/xrpl/test/integration/transactions/offerCreate.test.ts(2 hunks)packages/xrpl/test/integration/transactions/trustSet.test.ts(1 hunks)packages/xrpl/test/models/trustSet.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/xrpl/HISTORY.md
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: integration (22.x)
- GitHub Check: snippets (22.x)
- GitHub Check: unit (18.x)
- GitHub Check: integration (20.x)
- GitHub Check: snippets (20.x)
- GitHub Check: integration (18.x)
- GitHub Check: browser (18.x)
- GitHub Check: snippets (18.x)
🔇 Additional comments (2)
.ci-config/rippled.cfg (1)
191-191: Verify amendment dependency with C++ implementationThe PR objectives state that this PR should not be merged until the C++ implementation is integrated into the rippled project. We should ensure that enabling the
DeepFreezeamendment here is safe and won't cause issues if the C++ implementation isn't ready.Run this script to check the amendment status in rippled:
packages/xrpl/src/models/transactions/trustSet.ts (1)
33-36: Verify bit flag values with rippled implementationThe bit flag values (
0x00400000and0x00800000) for deep-freeze must match the C++ implementation in rippled.Run this script to verify the flag values:
Also applies to: 96-99
✅ Verification successful
Deep-freeze flag values are correctly implemented
The values
0x00400000(tfSetDeepFreeze) and0x00800000(tfClearDeepFreeze) are correctly implemented as part of the XLS-77d Deep-Freeze amendment. These values are consistently used across the codebase and validated through integration tests. While these bit positions are reused in other transaction and ledger entry types, this is a common practice in XRPL as each type maintains its own separate flag namespace.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify deep-freeze flag values match rippled implementation # Check flag definitions in rippled gh api repos/ripple/rippled/contents/src/ripple/protocol/TxFlags.h -q '.content' | base64 -d | grep -A 5 "tfSetDeepFreeze"Length of output: 217
Script:
#!/bin/bash # Try multiple approaches to verify the flag values # 1. Fetch rippled implementation using curl curl -s https://raw.githubusercontent.com/ripple/rippled/develop/src/ripple/protocol/TxFlags.h | grep -A 5 "tfSetDeepFreeze" # 2. Search for tests or documentation about these flags rg -i "deep.*freeze" -A 5 -B 5 # 3. Look for any validation of these specific hex values rg "0x00400000|0x00800000" -A 2 -B 2Length of output: 12337
Script:
#!/bin/bash # Look for XLS-77d documentation and specs rg -i "XLS-77d|deep.?freeze" --type md # Check if there are any conflicting uses of these specific values rg "0x00400000|0x00800000" --type ts --context 3Length of output: 3790
| it( | ||
| 'Create a Deep-Frozen trust line', | ||
| async () => { | ||
| assert(wallet2 != null) | ||
| // preemptively deep-freeze a trust line with the specified counter-party/currency-code | ||
| const tx: TrustSet = { | ||
| TransactionType: 'TrustSet', | ||
| Account: testContext.wallet.classicAddress, | ||
| LimitAmount: { | ||
| currency: 'USD', | ||
| issuer: wallet2.classicAddress, | ||
| value: '10', | ||
| }, | ||
| Flags: { | ||
| tfSetFreeze: true, | ||
| tfSetDeepFreeze: true, | ||
| }, | ||
| } | ||
|
|
||
| const response = await testTransaction( | ||
| testContext.client, | ||
| tx, | ||
| testContext.wallet, | ||
| ) | ||
| assert.equal(response.result.engine_result, 'tesSUCCESS') | ||
|
|
||
| // assert that the trust line is deep-frozen | ||
| const trustLine = await testContext.client.request({ | ||
| command: 'account_lines', | ||
| account: testContext.wallet.classicAddress, | ||
| }) | ||
|
|
||
| // assert that the TrustLine is deep-frozen | ||
| assert.equal(trustLine.result.lines[0].freeze, true) | ||
|
|
||
| // Keshava: ensure that account_lines RPC response contains a deep_freeze flag | ||
| // assert.equal(trustLine.result.lines[0].deep_freeze, true) | ||
| }, | ||
| TIMEOUT, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test cases for high/low side deep freeze behavior.
The test suite should verify:
- Deep freeze behavior from both high and low sides of the trust line
- Interaction between regular freeze and deep freeze
- Clearing of deep freeze flags
it('Deep freeze from high side', async () => {
// Similar test but with wallet2 setting the deep freeze
})
it('Clear deep freeze', async () => {
// Test clearing the deep freeze flag
})
it('Interaction between regular and deep freeze', async () => {
// Test combinations of regular and deep freeze flags
})There was a problem hiding this comment.
these cases are covered by the cpp tests. they are not required in this repository
There was a problem hiding this comment.
@ckeshava Understood, thanks for clarifying that these test cases are already covered in the C++ implementation.
✏️ Learnings added
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2873
File: packages/xrpl/test/integration/transactions/trustSet.test.ts:0-0
Timestamp: 2025-01-31T17:46:25.234Z
Learning: For the XRPL implementation, extensive test cases for deep freeze behavior (high/low side interactions, clearing flags, etc.) are maintained in the C++ implementation and don't need to be duplicated in the JavaScript implementation.
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.
| // Keshava: ensure that account_lines RPC response contains a deep_freeze flag | ||
| // assert.equal(trustLine.result.lines[0].deep_freeze, true) |
There was a problem hiding this comment.
💡 Codebase verification
Remove the commented deep_freeze assertion
The deep_freeze flag is not part of the XRPL account_lines API. The trust line freeze functionality is handled by the documented freeze and freeze_peer flags. This commented assertion should be removed as it references a non-existent feature.
🔗 Analysis chain
Clarify the status of the commented assertion.
The commented assertion suggests that the deep_freeze flag in the account_lines RPC response might be pending implementation. This should be either:
- Implemented and uncommented if the feature is ready
- Removed if the feature is not planned
- Converted to a TODO comment with a clear explanation if it's pending
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if deep_freeze flag is implemented in account_lines RPC
rg -A 5 "deep_freeze.*account_lines"
Length of output: 38
Script:
#!/bin/bash
# Search for deep_freeze usage in the codebase
rg "deep_freeze" -B 2 -A 2
# Search for account_lines test assertions
rg "account_lines.*assert" -B 2 -A 2
# Check documentation or type definitions for account_lines
fd "account.*lines" --type f --exec cat {}
Length of output: 83376
There was a problem hiding this comment.
@ckeshava Thank you for fixing this! The changes look good, and I appreciate the addition of proper deep-freeze verification using the account_objects RPC.
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Fetch the specific commit changes
git show c9ecd33eb88848d25d1dca2a24c5270ac356cb04
Length of output: 3745
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| /** Deep-Freeze the trust line -- disallow sending and recieving the said IssuedCurrency */ | ||
| tfSetDeepFreeze = 0x00400000, | ||
| /** Clear a Deep-Frozen trust line */ | ||
| tfClearDeepFreeze = 0x00800000, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding validation for mutually exclusive flags
The deep-freeze flags (tfSetDeepFreeze and tfClearDeepFreeze) are mutually exclusive, similar to the existing freeze flags. Consider adding validation to prevent setting both flags simultaneously.
Here's a suggested implementation:
function validateTrustSet(tx: Record<string, unknown>): void {
// ... existing validation ...
if (typeof tx.Flags === 'object') {
const flags = tx.Flags as TrustSetFlagsInterface
if (flags.tfSetDeepFreeze && flags.tfClearDeepFreeze) {
throw new ValidationError(
'TrustSet: cannot set both tfSetDeepFreeze and tfClearDeepFreeze flags',
)
}
}
}There was a problem hiding this comment.
there are many validation rules associated with the flags of trust-set transaction. It is not informative if certain rules are cherry-picked for implementation
There was a problem hiding this comment.
@ckeshava Understood, I'll retract the suggestion. You make a valid point about the importance of comprehensive validation rather than selective implementation.
✏️ Learnings added
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2873
File: packages/xrpl/src/models/transactions/trustSet.ts:33-36
Timestamp: 2025-01-08T13:08:52.450Z
Learning: For trust-set transactions in XRPL, validation rules for flags should be implemented comprehensively rather than cherry-picking specific rules, as there are many interdependent validation rules associated with these flags.
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.
I'd actually agree with this one, seems like an important check we should add. We aren't responsible for core logic like determining if a trustline is frozen before deep freezing, but surface level checks should be handled here to best catch invalid transactions like this
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.
@achowdhry-ripple I'm not entirely convinced that this is a feasible suggestion. There are many combination of invalid flags. For instance: tfSetDeepFreeze | tfClearFreeze, tfSetFreeze | tfClearDeepFreeze ... 2 more such combinations.
We would need to enumerate all such possibilities (the above is not an exhaustive list). It also necessitates defining a mask of valid flags for easier validation.
This task goes out of scope of the current PR and would be a repetition of the cpp code.
|
@shawnxie999 If you have any free time, please review this PR |
|
just a heads up I'll only be able to start reviewing this PR end of next week - if that's fine with you. doesn't want to be a blocker if this PR is urgent |
no worries, that is fine |
packages/xrpl/test/integration/transactions/offerCreate.test.ts
Outdated
Show resolved
Hide resolved
| tfSetDeepFreeze?: boolean | ||
| /** Clear a Deep-Frozen trust line */ | ||
| tfClearDeepFreeze?: boolean |
There was a problem hiding this comment.
could also possibly add a validation check that tfSetDeepFreeze and tfClearDeepFreeze should not be set in the same transaction? And possibly add a unit test for this.
unrelated to this PR, but a similar validation can be added for tfSetFreeze and tfClearFreeze to make sure they aren't set at the same time (and a unit test).
There was a problem hiding this comment.
Hmm, in the worst case, erring users will be greeted with a temINVALID_FLAG error here: https://github.com/XRPLF/rippled/blob/1b75dc8bcd536ff09a4d96064725c016e0df7293/src/xrpld/app/tx/detail/SetTrust.cpp#L42 (Users do not lose any transaction fees)
I have elaborated here as well: #2873 (comment)
We will need to replicate the tfTrustSetMask variable to do a complete validation. It feels a bit of an overkill.
packages/xrpl/test/integration/transactions/offerCreate.test.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Shawn Xie <35279399+shawnxie999@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/xrpl/test/integration/transactions/offerCreate.test.ts (3)
21-21: Consider renaming the wallet variable for clarity.Based on the past review comments, consider renaming
wallet_deep_freeze_trustlineto be more specific about its purpose, e.g.,deepFreezeTestWallet.Also applies to: 25-30
26-26: Document the ESLint disable comment.Please add a more detailed explanation for disabling the
require-atomic-updatesrule. While you mentioned that the race condition doesn't matter, it would be helpful to explain why it's safe to ignore in this context.
65-111: Enhance test coverage and documentation.The test case is well-structured, but consider these improvements:
- Add JSDoc comments to document the test scenario and expected behavior
- Add assertions to verify the trust line state after setting the deep-freeze
- Consider adding cleanup code to reset the trust line state after the test
Here's an example of how to enhance the test:
+ /** + * Verifies that OfferCreate transactions fail when using a deep-frozen trust line. + * + * Steps: + * 1. Set up a trust line with deep-freeze flags + * 2. Verify the trust line state + * 3. Attempt to create an offer using the frozen trust line + * 4. Verify the transaction fails with tecFROZEN + * 5. Clean up the trust line state + */ it( 'OfferCreate with Deep-Frozen trustline must fail', async () => { assert(wallet_deep_freeze_trustline != null) // deep-freeze the trust line const trust_set_tx: TrustSet = { // ... existing code ... } await testTransaction( testContext.client, trust_set_tx, testContext.wallet, ) + // Verify trust line state + const accountLines = await testContext.client.request({ + command: 'account_lines', + account: testContext.wallet.classicAddress, + peer: wallet_deep_freeze_trustline.classicAddress, + }) + assert.isTrue( + accountLines.result.lines?.[0].freeze, + 'Trust line should be frozen', + ) + assert.isTrue( + accountLines.result.lines?.[0].deep_freeze, + 'Trust line should be deep-frozen', + ) const offer_create_tx: OfferCreate = { // ... existing code ... } const response = await submitTransaction({ // ... existing code ... }) assert.equal(response.result.engine_result, 'tecFROZEN') + // Clean up: Clear the deep-freeze state + await testTransaction( + testContext.client, + { + ...trust_set_tx, + Flags: { + tfClearFreeze: true, + }, + }, + testContext.wallet, + ) }, TIMEOUT, )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/xrpl/test/integration/transactions/offerCreate.test.ts(2 hunks)packages/xrpl/test/integration/transactions/trustSet.test.ts(1 hunks)packages/xrpl/test/models/trustSet.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/xrpl/test/models/trustSet.test.ts
- packages/xrpl/test/integration/transactions/trustSet.test.ts
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: unit (22.x)
- GitHub Check: integration (22.x)
- GitHub Check: unit (20.x)
- GitHub Check: snippets (22.x)
- GitHub Check: integration (20.x)
- GitHub Check: unit (18.x)
- GitHub Check: snippets (20.x)
- GitHub Check: integration (18.x)
- GitHub Check: snippets (18.x)
- GitHub Check: browser (18.x)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
packages/xrpl/test/integration/transactions/offerCreate.test.ts (1)
3-3: LGTM! Import statements are well-organized.The new imports are correctly structured and necessary for implementing the deep-freeze functionality.
Also applies to: 10-14
Co-authored-by: Shawn Xie <35279399+shawnxie999@users.noreply.github.com>
Co-authored-by: Shawn Xie <35279399+shawnxie999@users.noreply.github.com>
Co-authored-by: Shawn Xie <35279399+shawnxie999@users.noreply.github.com>
High Level Overview of Change
This PR implements the Deep-Freeze spec: XRPLF/XRPL-Standards#220
CPP implementation: https://github.com/XRPLF/rippled/pull/5187/files#
Note: Please do not merge this PR until the CPP implementation is merged into the develop branch of rippled.
Type of Change
Did you update HISTORY.md?
Test Plan
New integration tests have been added to validate the Deep-Freeze behavior. Unit tests have been modified to incorporate deep-frozen trust lines.
Future Tasks
Once the behavior of Checks is better understood with the deep-frozen trust lines, additional integration tests can be added. However, they introduce non-trivial complexity in the code. Furthermore, such tests are already covered by the cpp implementaion.