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
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. The modifications include updating configuration files, extending enum definitions, 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
})| // 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
| /** 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.
|
@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 |
| if (!wallet2) { | ||
| // eslint-disable-next-line require-atomic-updates -- race condition doesn't really matter | ||
| wallet2 = await generateFundedWallet(testContext.client) | ||
| } |
There was a problem hiding this comment.
Better practice would be to put this in a beforeAll
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.