Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deep Freeze XLS-77d #2873

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .ci-config/rippled.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -188,3 +188,4 @@ fixNFTokenPageLinks
fixInnerObjTemplate2
fixEnforceNFTokenTrustline
fixReducedOffersV2
DeepFreeze
3 changes: 3 additions & 0 deletions packages/xrpl/HISTORY.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ Subscribe to [the **xrpl-announce** mailing list](https://groups.google.com/g/xr

## Unreleased Changes

### Added
* Support for XLS-77d Deep-Freeze amendment

## 4.1.0 (2024-12-23)

### Added
Expand Down
4 changes: 4 additions & 0 deletions packages/xrpl/src/models/ledger/RippleState.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,4 +77,8 @@ export enum RippleStateFlags {
lsfHighFreeze = 0x00800000,
// True, trust line to AMM. Used by client apps to identify payments via AMM.
lsfAMMNode = 0x01000000,
// True, low side has set deep freeze flag
lsfLowDeepFreeze = 0x02000000,
// True, high side has set deep freeze flag
lsfHighDeepFreeze = 0x04000000,
}
8 changes: 8 additions & 0 deletions packages/xrpl/src/models/transactions/trustSet.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ export enum TrustSetFlags {
tfSetFreeze = 0x00100000,
/** Unfreeze the trust line. */
tfClearFreeze = 0x00200000,
/** Deep-Freeze the trust line -- disallow sending and recieving the said IssuedCurrency */
Copy link
Collaborator

@shawnxie999 shawnxie999 Jan 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/** Deep-Freeze the trust line -- disallow sending and recieving the said IssuedCurrency */
/** Deep-Freeze the trustline -- disallow sending and receiving the said IssuedCurrency */

typo

tfSetDeepFreeze = 0x00400000,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be convenient to add a comment along the lines of

// Allowed only if the trustline is already regularly frozen, or if tfSetFreeze is set in the same transaction.

/** Clear a Deep-Frozen trust line */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/** Clear a Deep-Frozen trust line */
/** Clear a Deep-Frozen trustline */

tfClearDeepFreeze = 0x00800000,
Comment on lines +33 to +36
Copy link

@coderabbitai coderabbitai bot Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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',
      )
    }
  }
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

}

/**
Expand Down Expand Up @@ -89,6 +93,10 @@ export interface TrustSetFlagsInterface extends GlobalFlags {
tfSetFreeze?: boolean
/** Unfreeze the trust line. */
tfClearFreeze?: boolean
/** Deep-Freeze the trust line -- disallow sending and recieving the said IssuedCurrency */
Copy link
Collaborator

@shawnxie999 shawnxie999 Jan 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/** Deep-Freeze the trust line -- disallow sending and recieving the said IssuedCurrency */
/** Deep-Freeze the trustline -- disallow sending and receiving the said IssuedCurrency */

tfSetDeepFreeze?: boolean
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

/** Clear a Deep-Frozen trust line */
tfClearDeepFreeze?: boolean
Comment on lines +97 to +99
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

}

/**
Expand Down
68 changes: 64 additions & 4 deletions packages/xrpl/test/integration/transactions/offerCreate.test.ts
Original file line number Diff line number Diff line change
@@ -1,24 +1,36 @@
import { assert } from 'chai'

import { OfferCreate } from '../../../src'
import { OfferCreate, TrustSet, Wallet } from '../../../src'
import serverUrl from '../serverUrl'
import {
setupClient,
teardownClient,
type XrplIntegrationTestContext,
} from '../setup'
import { testTransaction } from '../utils'
import {
testTransaction,
generateFundedWallet,
submitTransaction,
} from '../utils'

// how long before each test case times out
const TIMEOUT = 20000

describe('OfferCreate', function () {
let testContext: XrplIntegrationTestContext
let wallet_deep_freeze_trustline: Wallet | undefined

beforeEach(async () => {
beforeAll(async () => {
testContext = await setupClient(serverUrl)
if (!wallet_deep_freeze_trustline) {
// eslint-disable-next-line require-atomic-updates -- race condition doesn't really matter
wallet_deep_freeze_trustline = await generateFundedWallet(
testContext.client,
)
}
})
afterEach(async () => teardownClient(testContext))

afterAll(async () => teardownClient(testContext))

it(
'base',
Expand Down Expand Up @@ -49,4 +61,52 @@ describe('OfferCreate', function () {
},
TIMEOUT,
)

it(
'OfferCreate with Deep-Frozen trust-line must fail',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'OfferCreate with Deep-Frozen trust-line must fail',
'OfferCreate with Deep-Frozen trustline must fail',

async () => {
assert(wallet_deep_freeze_trustline != null)

// deep-freeze the trust line
const trust_set_tx: TrustSet = {
TransactionType: 'TrustSet',
Account: testContext.wallet.classicAddress,
LimitAmount: {
currency: 'USD',
issuer: wallet_deep_freeze_trustline.classicAddress,
value: '10',
},
Flags: {
tfSetFreeze: true,
tfSetDeepFreeze: true,
},
}

await testTransaction(
testContext.client,
trust_set_tx,
testContext.wallet,
)

const offer_create_tx: OfferCreate = {
TransactionType: 'OfferCreate',
Account: testContext.wallet.classicAddress,
TakerGets: '13100000',
TakerPays: {
currency: 'USD',
issuer: wallet_deep_freeze_trustline.classicAddress,
value: '10',
},
}

const response = await submitTransaction({
client: testContext.client,
transaction: offer_create_tx,
wallet: testContext.wallet,
})

assert.equal(response.result.engine_result, 'tecFROZEN')
},
TIMEOUT,
)
})
41 changes: 41 additions & 0 deletions packages/xrpl/test/integration/transactions/trustSet.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,4 +85,45 @@ describe('TrustSet', function () {
},
TIMEOUT,
)

it(
'Create a Deep-Frozen trust line',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'Create a Deep-Frozen trust line',
'Create a Deep-Frozen trustline',

async () => {
assert(wallet2 != null)
// preemptively deep-freeze a trust line with the specified counter-party/currency-code
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// preemptively deep-freeze a trust line with the specified counter-party/currency-code
// preemptively deep-freeze a trustline 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// assert that the trust line is deep-frozen
// assert that the trustline is deep-frozen

const trustLine = await testContext.client.request({
command: 'account_lines',
account: testContext.wallet.classicAddress,
})

// assert that the TrustLine is deep-frozen
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// assert that the TrustLine is deep-frozen
// 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)
Comment on lines +124 to +125
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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:

  1. Implemented and uncommented if the feature is ready
  2. Removed if the feature is not planned
  3. 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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alternatively (or do both), you can check for account_objects and check manually that the trustline object has toggled the deep freeze flag

},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not necessary - but also could possibly trying to send a payment to the deep-frozen account, which should fail. (since payment txn is the main transaction that deep freeze is affecting)

TIMEOUT,
)
Comment on lines +89 to +128
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add test cases for high/low side deep freeze behavior.

The test suite should verify:

  1. Deep freeze behavior from both high and low sides of the trust line
  2. Interaction between regular freeze and deep freeze
  3. 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
})

})
5 changes: 5 additions & 0 deletions packages/xrpl/test/models/trustSet.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,11 @@ describe('TrustSet', function () {
},
QualityIn: 1234,
QualityOut: 4321,
// an example of deep-frozen trust line
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// an example of deep-frozen trust line
// an example of deep-frozen trustline

Flags: {
tfSetFreeze: true,
tfSetDeepFreeze: true,
},
} as any
})

Expand Down
Loading