Mark AssetsAvailable, AssetsTotal and LossUnrealized of Vault object as Optional#3153
Mark AssetsAvailable, AssetsTotal and LossUnrealized of Vault object as Optional#3153
Conversation
WalkthroughThis PR makes three properties of the Vault interface optional (AssetsTotal, AssetsAvailable, LossUnrealized) across type definitions, updates test assertions to handle undefined values using nullish coalescing operators, and documents the changes in the release notes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
| * Amount of assets currently available for withdrawal. | ||
| */ | ||
| AssetsAvailable: string | ||
| AssetsAvailable?: string |
There was a problem hiding this comment.
It seems like a breaking change is introduced in rippled that makes these properties optional that were once required. If someone if accessing them directly as we did in integration tests, it would break their code.
Not to block this, but should we confirm this in cpp-informal or xrpl-lending-protocol-core slack channel?
There was a problem hiding this comment.
Thanks for bringing this up. I asked in #cpp-informal. The conclusion is this is fine since SAV is not even up for voting yet and breaking changes are allowed.
There was a problem hiding this comment.
Saw the note from Mayukha, since we are releasing library even before the amendment goes live on Mainnet, we should allow such breaking changes as bug fixes OR keep releasing, the features as release candidates and only release a stable version once the amendment is live on mainnet (to follow Semver). Some thoughts...
There was a problem hiding this comment.
What you said makes sense. Feel free to champion this, and we should adopt it if it brings benefits to the team and the community.
There was a problem hiding this comment.
we are releasing library even before the amendment goes live on Mainnet, we should allow such breaking changes as bug fixes
Where can people use the client library features for Single-Asset-Vault? Apart from standalone nodes on their local nodes, is it available for use on public networks? (The XRPLF developers have the right to introduce breaking changes into the devnet, because features are experimental, by definition on the devnet.)
ckeshava
left a comment
There was a problem hiding this comment.
Hello Kuan, the changes proposed in this file aid the passing of integration tests related to Single-Asset-Vault. Is this PR blocking the execution of other integration tests as well?
| * The total value of the vault. | ||
| */ | ||
| AssetsTotal: string | ||
| AssetsTotal?: string |
There was a problem hiding this comment.
Note: These fields have been marked as "Default", not "Optional". Hence, we need to introduce a Default value for these fields. I'm not sure if there is a precedent in the xrpl.js library, we'll need to look into it.
There was a problem hiding this comment.
Those fields of the Vault ledger entry were updated from soeREQUIRED to soeDEFAULT, and, by definition, soeDEFAULT is optional, if present, must not have default value. The correct TypeScript representation is simply making the field optional with ?, which is what we've done in this library, e.g., AccountRoot.AccountTxnID?, AccountRoot.AMMID?, AccountRoot.BurnedNFTokens?, AccountRoot.BurnedNFTokens?
| ) | ||
| assert.equal( | ||
| vault.AssetsTotal, | ||
| vault.AssetsTotal ?? '0', |
There was a problem hiding this comment.
Why is the test file supplying a default value for these fields? The default value of 0 must have been provided by rippled automatically.
It is preferable to remove the ?? operator because it will inform readers about the choices for default values.
| vault.AssetsTotal ?? '0', | |
| vault.AssetsTotal, |
There was a problem hiding this comment.
The default value of 0 must have been provided by rippled automatically.
This is no longer true, and why those tests are failing. Without the change, we are performing assert.equal(vault.AssetsTotal=undefined, '0', 'New Vault should have zero total assets')
| ### Fixed | ||
| * Update ripple-binary-codec to 2.5.1 to address serialization/deserialization issues in `Issue` serialized type for `MPTIssue`. | ||
| * Better faucet error handling | ||
| * Mark the `AssetsAvailable`, `AssetsTotal`, and `LossUnrealized` fields of the Vault object as optional. |
There was a problem hiding this comment.
| * Mark the `AssetsAvailable`, `AssetsTotal`, and `LossUnrealized` fields of the Vault object as optional. | |
| * Mark the `AssetsAvailable`, `AssetsTotal`, and `LossUnrealized` fields of the Vault object as Default. |
There was a problem hiding this comment.
I don't think we need to make this change. In rippled, soeDEFAULT is an internal serialization style. In xrpl.js/Typescript, fields are either required or optional (marked with ?). There is no concept of "Default" fields in TypeScript.
| * Total amount of assets in the vault. | ||
| */ | ||
| AssetsTotal: string | ||
| AssetsTotal?: string |
There was a problem hiding this comment.
Why are these fields marked as Optional in the vaultInfo RPC method? As per my understanding, the vaultInfo RPC response prints the Default values for these fields, even if the genesis VaultCreate transaction did not explicitly specify these fields.
In your experiments, did you observe that vaultInfo does not print these fields?
There was a problem hiding this comment.
In your experiments, did you observe that vaultInfo does not print these fields?
Correct. The existing integration tests are failing because vaultInfo RPC doesn't include those fields in the response when they have default values. This match the definition of an soeDEFAULT field, which is optional, if present, must not have default value. In other words, If they have the default values, they won't be present in rippled, and thus Vault related RPC responses.
ckeshava
left a comment
There was a problem hiding this comment.
Thanks for the clarification: I was under the misapprehension that rippled stores ledger-object fields with "Default" values, that is not true.
I have created another ticket to track the misconfiguration of other ledger-objects, with respect to the DEFAULT parameter. #3154
…as Optional (XRPLF#3153) * Mark AssetsAvailable, AssetsTotal and LossUnrealized of Vault object as optional
* release pipeline change for beta release * release pipeline change for beta release * release pipeline change for beta release * update workflow for beta release * update workflow for beta release * update workflow for beta release * update workflow for beta release * update workflow for beta release * update workflow for beta release * update workflow for beta release * update workflow for beta release * update workflow for beta release * update workflow for beta release * update workflow for beta release * update workflow for beta release * update workflow for beta release * update workflow for beta release * update workflow for beta release * update workflow for beta release * update workflow for beta release * update workflow for beta release * update workflow for beta release * update workflow for beta release * update workflow for beta release * update workflow for beta release * update workflow for beta release * update workflow for beta release * update workflow for beta release * fix sending failure notification * fix sending failure notification * fix sending failure notification * revert changes * revert changes * revert changes * test release 4.4.21 * resove conflict * resove conflict * resove conflict * resove conflict * revert slack channel * fix git ref for generate document * resolve beta release PR comments * update slack channel * fix beta dist tag * update RELEASE.md * update RELEASE.md * resolve comments on RELEASE.md * update RELEASE.md and only unit test for beta release * only run unit tests for beta releaes * only run unit tests for beta releaes * change back slack channel * fix owasp project name * add back PR auto creation * test release after add back pr auto creation * auto raise PR as draft * auto raise PR as draft * auto raise PR as draft * auto raise PR as draft * auto raise PR as draft * update RELEASE.md * revert slack channel * update integration/faucet trigger * fix: better formatting handling in definitions script (#3123) * add newlines * fix typo * Update definitions.json * fix test * fix: improve faucet error handling (#3118) * improve faucet erroring * update history * fix tests * fix history * test: make connections.test.ts run faster (#3113) * Change dependabot frequency to monthly (#3139) * update depandabot frequency * change freq to monthly * build(deps-dev): bump typedoc from 0.28.5 to 0.28.15 (#3147) Bumps [typedoc](https://github.com/TypeStrong/TypeDoc) from 0.28.5 to 0.28.15. - [Release notes](https://github.com/TypeStrong/TypeDoc/releases) - [Changelog](https://github.com/TypeStrong/typedoc/blob/master/CHANGELOG.md) - [Commits](TypeStrong/typedoc@v0.28.5...v0.28.15) --- updated-dependencies: - dependency-name: typedoc dependency-version: 0.28.15 dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Mark AssetsAvailable, AssetsTotal and LossUnrealized of Vault object as Optional (#3153) * Mark AssetsAvailable, AssetsTotal and LossUnrealized of Vault object as optional * Add support for lending protocol xls-66d (#3138) * WIP commit * WIP commit * WIP commit * add LoanBrokerSet transaction * update definitions.json * update history * add LoanBrokerCoverDeposit * add LoanBrokerCoverWithdraw * add LoanBrokerCoverClawback * add LoanSet transaction * add loan transactions * WIP commit * WIP commit * WIP commit * fix models * WIP commit * WIP commit * WIP commit * WIP commit * WIP commit * WIP commit * WIP commit * WIP commit * fix types * fix autofill for VaultCreate * implement autofill for LoanSet * WIP commit * Mark AssetsAvailable, AssetsTotal and LossUnrealized of Vault object as optional * Update HISTORY.md * assert objects in test cases * test autofill for LoanSet tx * improve warning message * change to console.warn * address codarabbit suggestions * add unit test for encodeForMultisigning with SigningPubKey present * address code-review comments and LoanSet autofill default * add conditions on flags * address codarabbit suggestions * enable PermissionDelegationV1_1 --------- Co-authored-by: Kuan Lin <[email protected]> * feat: export more helper functions (#3157) * Release 2.6.0: release-rbc-2.6.0 → main (#3159) update package version Co-authored-by: Raj Patel <[email protected]> * Release 4.5.0: release-xrpl-4.5.0 → main (#3161) update package version Co-authored-by: Raj Patel <[email protected]> * build(deps-dev): bump expect from 29.7.0 to 30.2.0 (#3148) Bumps [expect](https://github.com/jestjs/jest/tree/HEAD/packages/expect) from 29.7.0 to 30.2.0. - [Release notes](https://github.com/jestjs/jest/releases) - [Changelog](https://github.com/jestjs/jest/blob/main/CHANGELOG.md) - [Commits](https://github.com/jestjs/jest/commits/v30.2.0/packages/expect) --- updated-dependencies: - dependency-name: expect dependency-version: 30.2.0 dependency-type: direct:development update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Upgrade lerna to v8 (#3156) update the lerna version * Set dependabot version update frequency to quarterly (#3169) change freq * build(deps-dev): bump @eslint/js from 9.35.0 to 9.39.2 (#3170) Bumps [@eslint/js](https://github.com/eslint/eslint/tree/HEAD/packages/js) from 9.35.0 to 9.39.2. - [Release notes](https://github.com/eslint/eslint/releases) - [Commits](https://github.com/eslint/eslint/commits/v9.39.2/packages/js) --- updated-dependencies: - dependency-name: "@eslint/js" dependency-version: 9.39.2 dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * build(deps-dev): bump @types/lodash from 4.17.20 to 4.17.21 (#3178) Bumps [@types/lodash](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/lodash) from 4.17.20 to 4.17.21. - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/lodash) --- updated-dependencies: - dependency-name: "@types/lodash" dependency-version: 4.17.21 dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * build(deps-dev): bump webpack from 5.102.0 to 5.104.1 (#3166) Bumps [webpack](https://github.com/webpack/webpack) from 5.102.0 to 5.104.1. - [Release notes](https://github.com/webpack/webpack/releases) - [Changelog](https://github.com/webpack/webpack/blob/main/CHANGELOG.md) - [Commits](webpack/webpack@v5.102.0...v5.104.1) --- updated-dependencies: - dependency-name: webpack dependency-version: 5.104.1 dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * build(deps-dev): bump ts-jest from 29.4.1 to 29.4.6 (#3162) Bumps [ts-jest](https://github.com/kulshekhar/ts-jest) from 29.4.1 to 29.4.6. - [Release notes](https://github.com/kulshekhar/ts-jest/releases) - [Changelog](https://github.com/kulshekhar/ts-jest/blob/main/CHANGELOG.md) - [Commits](kulshekhar/ts-jest@v29.4.1...v29.4.6) --- updated-dependencies: - dependency-name: ts-jest dependency-version: 29.4.6 dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * build(deps-dev): bump eslint-plugin-n from 17.21.3 to 17.23.1 (#3143) Bumps [eslint-plugin-n](https://github.com/eslint-community/eslint-plugin-n) from 17.21.3 to 17.23.1. - [Release notes](https://github.com/eslint-community/eslint-plugin-n/releases) - [Changelog](https://github.com/eslint-community/eslint-plugin-n/blob/master/CHANGELOG.md) - [Commits](eslint-community/eslint-plugin-n@v17.21.3...v17.23.1) --- updated-dependencies: - dependency-name: eslint-plugin-n dependency-version: 17.23.1 dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * build(deps-dev): bump typescript-eslint from 8.39.0 to 8.52.0 (#3173) * build(deps-dev): bump typescript-eslint from 8.39.0 to 8.52.0 Bumps [typescript-eslint](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/typescript-eslint) from 8.39.0 to 8.52.0. - [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases) - [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/typescript-eslint/CHANGELOG.md) - [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v8.52.0/packages/typescript-eslint) --- updated-dependencies: - dependency-name: typescript-eslint dependency-version: 8.52.0 dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> * build(deps-dev): bump typescript-eslint from 8.39.0 to 8.52.0 (#3181) * Initial plan * Initial plan for fixing typescript-eslint linting issues Co-authored-by: mvadari <[email protected]> * Fix typescript-eslint linting issues from 8.52.0 upgrade Co-authored-by: mvadari <[email protected]> * Disable import/no-unresolved for isomorphic test files using module subpaths Co-authored-by: mvadari <[email protected]> * Fix st-number.ts logic and revert import/no-unresolved disable Co-authored-by: mvadari <[email protected]> * Update typescript-eslint version in package.json to 8.52.0 Co-authored-by: mvadari <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: mvadari <[email protected]> --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Copilot <[email protected]> Co-authored-by: mvadari <[email protected]> Co-authored-by: Mayukha Vadari <[email protected]> * build(deps-dev): bump webpack-bundle-analyzer from 4.10.2 to 5.1.0 (#3171) Bumps [webpack-bundle-analyzer](https://github.com/webpack/webpack-bundle-analyzer) from 4.10.2 to 5.1.0. - [Changelog](https://github.com/webpack/webpack-bundle-analyzer/blob/main/CHANGELOG.md) - [Commits](webpack/webpack-bundle-analyzer@v4.10.2...v5.1.0) --- updated-dependencies: - dependency-name: webpack-bundle-analyzer dependency-version: 5.1.0 dependency-type: direct:development update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * build(deps): bump ws from 8.18.3 to 8.19.0 (#3172) Bumps [ws](https://github.com/websockets/ws) from 8.18.3 to 8.19.0. - [Release notes](https://github.com/websockets/ws/releases) - [Commits](websockets/ws@8.18.3...8.19.0) --- updated-dependencies: - dependency-name: ws dependency-version: 8.19.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * build(deps-dev): bump eslint from 9.35.0 to 9.39.2 (#3163) Bumps [eslint](https://github.com/eslint/eslint) from 9.35.0 to 9.39.2. - [Release notes](https://github.com/eslint/eslint/releases) - [Commits](eslint/eslint@v9.35.0...v9.39.2) --- updated-dependencies: - dependency-name: eslint dependency-version: 9.39.2 dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * build(deps-dev): bump jest-mock from 29.7.0 to 30.2.0 (#3174) Bumps [jest-mock](https://github.com/jestjs/jest/tree/HEAD/packages/jest-mock) from 29.7.0 to 30.2.0. - [Release notes](https://github.com/jestjs/jest/releases) - [Changelog](https://github.com/jestjs/jest/blob/main/CHANGELOG.md) - [Commits](https://github.com/jestjs/jest/commits/v30.2.0/packages/jest-mock) --- updated-dependencies: - dependency-name: jest-mock dependency-version: 30.2.0 dependency-type: direct:development update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * update trigger for integration test and faucet test --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Mayukha Vadari <[email protected]> Co-authored-by: Raj Patel <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Kuan Lin <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Chenna Keshava B S <[email protected]> Co-authored-by: Copilot <[email protected]> Co-authored-by: mvadari <[email protected]>
High Level Overview of Change
Mark the AssetsAvailable, AssetsTotal, and LossUnrealized fields of the Vault object as optional to match the behavior introduced by the recent rippled changes (git blame), which change those fields from
soeREQUIREDtosoeDEFAULT.Context of Change
Fix #3152
Type of Change
Did you update HISTORY.md?
Test Plan