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 10 commits into
base: main
Choose a base branch
from
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 */
ckeshava marked this conversation as resolved.
Show resolved Hide resolved
tfSetDeepFreeze = 0x00400000,
ckeshava marked this conversation as resolved.
Show resolved Hide resolved
/** Clear a Deep-Frozen trust line */
ckeshava marked this conversation as resolved.
Show resolved Hide resolved
tfClearDeepFreeze = 0x00800000,
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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

thats a good point, can leave this out

}

/**
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 */
ckeshava marked this conversation as resolved.
Show resolved Hide resolved
tfSetDeepFreeze?: boolean
ckeshava marked this conversation as resolved.
Show resolved Hide resolved
/** Clear a Deep-Frozen trust line */
tfClearDeepFreeze?: boolean
Comment on lines +99 to +101
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).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

}

/**
Expand Down
61 changes: 59 additions & 2 deletions packages/xrpl/test/integration/transactions/offerCreate.test.ts
Original file line number Diff line number Diff line change
@@ -1,22 +1,31 @@
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 wallet2: Wallet | undefined

beforeEach(async () => {
testContext = await setupClient(serverUrl)
if (!wallet2) {
// eslint-disable-next-line require-atomic-updates -- race condition doesn't really matter
wallet2 = await generateFundedWallet(testContext.client)
}
ckeshava marked this conversation as resolved.
Show resolved Hide resolved
})
afterEach(async () => teardownClient(testContext))

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

it(
'OfferCreate with Deep-Frozen trust-line must fail',
ckeshava marked this conversation as resolved.
Show resolved Hide resolved
async () => {
assert(wallet2 != null)

// deep-freeze the trust line
const trust_set_tx: TrustSet = {
TransactionType: 'TrustSet',
Account: testContext.wallet.classicAddress,
LimitAmount: {
currency: 'USD',
issuer: wallet2.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: wallet2.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',
ckeshava marked this conversation as resolved.
Show resolved Hide resolved
async () => {
assert(wallet2 != null)
// preemptively deep-freeze a trust line with the specified counter-party/currency-code
ckeshava marked this conversation as resolved.
Show resolved Hide resolved
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
ckeshava marked this conversation as resolved.
Show resolved Hide resolved
const trustLine = await testContext.client.request({
command: 'account_lines',
account: testContext.wallet.classicAddress,
})

// assert that the TrustLine is deep-frozen
ckeshava marked this conversation as resolved.
Show resolved Hide resolved
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)
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.

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

Choose a reason for hiding this comment

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

fixed in c9ecd33

Copy link

Choose a reason for hiding this comment

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

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

ckeshava marked this conversation as resolved.
Show resolved Hide resolved
},
ckeshava marked this conversation as resolved.
Show resolved Hide resolved
TIMEOUT,
)
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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

these cases are covered by the cpp tests. they are not required in this repository

Copy link

Choose a reason for hiding this comment

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

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

})
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
ckeshava marked this conversation as resolved.
Show resolved Hide resolved
Flags: {
tfSetFreeze: true,
tfSetDeepFreeze: true,
},
} as any
})

Expand Down
Loading