Conversation
|
""" WalkthroughThe changes update the Changes
Assessment against linked issues
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm warn EBADENGINE Unsupported engine { 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (7)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
Patel-Raj11
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/xrpl/src/models/ledger/AccountRoot.ts (1)
75-77: Docstring wording can be tightened to match the new field nameThe property name now includes “Token”, but the accompanying comment still reads a bit awkwardly (“The sequence that the account first minted an NFToken”). Consider re-phrasing for clarity:
- /** The sequence that the account first minted an NFToken */ + /** Sequence number of the first NFToken that this account minted. */packages/xrpl/test/integration/transactions/nftokenMint.test.ts (1)
52-53: Minor readability nit – de-structure the Sequence directlyPurely stylistic, but de-structuring the value makes the intent clearer:
- const mintTransactionSeq = response.result.tx_json.Sequence + const { Sequence: mintTransactionSeq } = response.result.tx_json
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/xrpl/HISTORY.md(1 hunks)packages/xrpl/src/models/ledger/AccountRoot.ts(1 hunks)packages/xrpl/test/integration/transactions/nftokenMint.test.ts(3 hunks)
🔇 Additional comments (1)
packages/xrpl/HISTORY.md (1)
7-9: Changelog entry looks good – accurately captures the fix and follows the existing formatting.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/xrpl/test/integration/transactions/nftokenMint.test.ts (2)
52-53: Minor: useconst { Sequence } = response.result.tx_jsonfor brevity.Purely cosmetic – feel free to ignore.
100-117: Edge-case: ledger state assertions assume a pristine wallet.
FirstNFTokenSequence === mintTransactionSeqandMintedNFTokens === 1are only true when the wallet has never minted before.
If this helper is re-used with a wallet that already owns NFTokens the assertions will fail.Consider loosening the expectations:
-assert.equal(accountData.MintedNFTokens, 1) +assert.isAtLeast(accountData.MintedNFTokens, 1)or generate / fund a fresh wallet inside the test to guarantee isolation (cheapest option on dev-nets).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/xrpl/HISTORY.md(1 hunks)packages/xrpl/src/models/ledger/AccountRoot.ts(1 hunks)packages/xrpl/test/integration/transactions/nftokenMint.test.ts(3 hunks)
🔇 Additional comments (3)
packages/xrpl/src/models/ledger/AccountRoot.ts (1)
73-78:Details
✅ Verification successful
Verify that all call-sites were migrated from
FirstNFTSequencetoFirstNFTokenSequence.The interface now exposes
FirstNFTokenSequence?: number, but the oldFirstNFTSequenceidentifier may still linger in helpers, parsers, or tests outside of this PR. A stale reference will silently compile (asany) or break at runtime when decoding ledger data.If any matches are returned, rename them or update the type import.
🏁 Script executed:
#!/bin/bash # Search the entire repo for the old identifier rg --fixed-strings 'FirstNFTSequence' -nLength of output: 16542
No lingering
FirstNFTSequencereferences in codeA repository-wide search found
FirstNFTSequenceonly in HISTORY.md and generated docs—not in any source files, helpers, parsers, or tests. All call-sites have been updated to useFirstNFTokenSequence.packages/xrpl/HISTORY.md (1)
7-9: Changelog entry looks good – no action needed.packages/xrpl/test/integration/transactions/nftokenMint.test.ts (1)
4-4: Nice catch adding a typed request object.Importing
AccountInfoRequestkeeps the test strongly typed and future-proof.
* fix AccountRoot ledger object * update HISTORY.md * fix import and lint errors * request a validated ledger
* update HISTORY files * enable SAV amendment * add VaultCreate tx and update autofill * build(deps-dev): bump karma from 6.4.3 to 6.4.4 (#2753) * build(deps): bump @scure/bip39 from 1.5.4 to 1.6.0 (#3004) * Fix AccountRoot ledger object (#3010) * fix AccountRoot ledger object * update HISTORY.md * fix import and lint errors * request a validated ledger * build(deps-dev): bump react from 19.0.0 to 19.1.0 (#2968) * upgrade ws dependency (#2940) Co-authored-by: achowdhry-ripple <achowdhry@ripple.com> * build(deps-dev): bump webpack from 5.98.0 to 5.99.9 (#3015) Bumps [webpack](https://github.com/webpack/webpack) from 5.98.0 to 5.99.9. - [Release notes](https://github.com/webpack/webpack/releases) - [Commits](webpack/webpack@v5.98.0...v5.99.9) --- updated-dependencies: - dependency-name: webpack dependency-version: 5.99.9 dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: achowdhry-ripple <achowdhry@ripple.com> Co-authored-by: Omar Khan <khancodegt@gmail.com> * add Vault ledger entry and base integ test * add rippled serialized type Number. update models & tests to support it * fix: add conditional check for `PermissionValue` definition (#3018) * add conditional check for PermissionValue * update HISTORY * remove release date * add unit test * ripple-binary-codec 2.4.1 patch release (#3019) * update HISTORY * update package.json files * update package-lock.json * test: Separate faucet tests from local integration tests (#2985) * refactor(tests): separate faucet tests from local integration tests * feat: add Faucet test workflow and documentation * fix: Add missing test:faucet script * fix: execute tests one time Co-authored-by: Raj Patel <rajp@ripple.com> * fix: remove docker steps from faucet test workflow * docs: update faucet tests section in CONTRIBUTING.md * fix: remove comment from contributing.md Co-authored-by: Raj Patel <rajp@ripple.com> * fix: remove test:all from root package Co-authored-by: Raj Patel <rajp@ripple.com> * Update CONTRIBUTING.md --------- Co-authored-by: Raj Patel <rajp@ripple.com> * update number test * debug * set alias Number to SerializedNumber in coreTypes * update fee in VaultCreate integ test * up the fee * up more * debug fee * update fee with comment * add vault_info RPC with integ test * remove unused var in test * use vault_id * update vaultInfo test * add owner and seq params to vault_info request * add DomainID to vault_info response * add XRPLNumber type and VaultSet tx * rename SerializedNumber to XRPLNumber * add VaultSet to integ test * refactor DEFAULT_VAULT_WITHDRAWAL_POLICY location * fix import bug * add VaultDeposit tx * add VaultDeposit to integ test * add VaultWithdraw tx * add VaultWithdraw to integ test * add VaultClawback tx * add VaultClawback to integ test * use 2 wallets for integ test * comment out VaultClawback tx in integ test * use IOU in integ test * add ClawbackAmount type and VaultClawback to integ test * add VaultDelete tx * remove isBigInt * add VaultDelete to integ test * rename to STNumber * comment out Amount in VaultClawback integ test * Revert "comment out Amount in VaultClawback integ test" This reverts commit 416c1e1. * comment out WithdrawalPolicy in VaultCreate integ test * refactor WithdrawalPolicy * cleanup * cleanup vars * update HISTORY * rename var to VAULT_DATA_MAX_BYTE_LENGTH * use STNumber class in static function * add VaultCreate to codec-fixtures * refactor unit tests * update HISTORY --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Raj Patel <rajp@ripple.com> Co-authored-by: Chenna Keshava B S <21219765+ckeshava@users.noreply.github.com> Co-authored-by: achowdhry-ripple <achowdhry@ripple.com> Co-authored-by: Achal Jhawar <35405812+achaljhawar@users.noreply.github.com>
3 participants
High Level Overview of Change
Fixes #3007
Context of Change
FirstNFTokenSequencefield.NFTokenMintis executed.Type of Change
Did you update HISTORY.md?
Test Plan