-
Notifications
You must be signed in to change notification settings - Fork 523
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
base: main
Are you sure you want to change the base?
Deep Freeze XLS-77d #2873
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -188,3 +188,4 @@ fixNFTokenPageLinks | |
fixInnerObjTemplate2 | ||
fixEnforceNFTokenTrustline | ||
fixReducedOffersV2 | ||
DeepFreeze |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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 */ | ||||||
tfSetDeepFreeze = 0x00400000, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would be convenient to add a comment along the lines of
|
||||||
/** Clear a Deep-Frozen trust line */ | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
tfClearDeepFreeze = 0x00800000, | ||||||
Comment on lines
+33
to
+36
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ( 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. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
|
||||||
} | ||||||
|
||||||
/** | ||||||
|
@@ -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 */ | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
tfSetDeepFreeze?: boolean | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could also possibly add a validation check that 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). |
||||||
} | ||||||
|
||||||
/** | ||||||
|
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', | ||||||
|
@@ -49,4 +61,52 @@ describe('OfferCreate', function () { | |||||
}, | ||||||
TIMEOUT, | ||||||
) | ||||||
|
||||||
it( | ||||||
'OfferCreate with Deep-Frozen trust-line must fail', | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
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, | ||||||
) | ||||||
}) |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -85,4 +85,45 @@ describe('TrustSet', function () { | |||||
}, | ||||||
TIMEOUT, | ||||||
) | ||||||
|
||||||
it( | ||||||
'Create a Deep-Frozen trust line', | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
async () => { | ||||||
assert(wallet2 != null) | ||||||
// preemptively deep-freeze a trust line with the specified counter-party/currency-code | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
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 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
const trustLine = await testContext.client.request({ | ||||||
command: 'account_lines', | ||||||
account: testContext.wallet.classicAddress, | ||||||
}) | ||||||
|
||||||
// assert that the TrustLine is deep-frozen | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Remove the commented deep_freeze assertion The 🔗 Analysis chainClarify the status of the commented assertion. The commented assertion suggests that the
🏁 Scripts executedThe 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. alternatively (or do both), you can check for |
||||||
}, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
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
}) |
||||||
}) |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -22,6 +22,11 @@ describe('TrustSet', function () { | |||||
}, | ||||||
QualityIn: 1234, | ||||||
QualityOut: 4321, | ||||||
// an example of deep-frozen trust line | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
Flags: { | ||||||
tfSetFreeze: true, | ||||||
tfSetDeepFreeze: true, | ||||||
}, | ||||||
} as any | ||||||
}) | ||||||
|
||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo