feat(models)!: гвард полей ledger-объектов и чистка моделей по ledger_entries.macro (10.11.0.0) - #75
Conversation
…_entries.macro
Третья и последняя поверхность конформанса: TestUTxFormatConformance держит
поля транзакций, TestULedgerFlagsConformance — флаги ledger-объектов, а поля
самих объектов не проверял никто. ledger_entries.macro — единственное место,
где протокол объявляет состав объекта (definitions.json несёт коды полей и
типы, но не списки), и пропущенное поле симптома не даёт: чтение объекта
проходит, значение просто теряется.
Гвард:
* вендоренный Fixtures/ledger_entries.macro с пином по sha. Пин на develop,
а не на тег (в отличие от LedgerFormats.h): модели по полям следуют develop,
и sfLEVersion существует только после 30.07 — тег объявил бы его выдумкой SDK
* диф в обе стороны + обязательная регистрация каждого объекта, поэтому новый
ledger-объект роняет сборку, а не пропускается
* четыре common-поля rippled (LedgerIndex, LedgerEntryType, Flags, Sponsor из
LedgerFormats::getCommonFields()) исключаются с обеих сторон, как это уже
сделано для commonFields в TxFormat-гварде; [JsonIgnore]-свойства не в счёт
* проверен мутацией: переименование JsonPropertyName даёт обе половины отчёта
Удалены свойства, которых нет в протоколе (BREAKING, без [Obsolete] — как при
удалении инертных ConnectionOptions в 10.10.0.0). Значение они держать не могли:
объект собирается по фиксированному SOTemplate. Проверено и на живой ноде, и по
четырём версиям (3.2.1, 3.3.0-b1, 3.3.0-rc1, develop) — нет нигде:
* LOVault.DomainID — positive control: VaultCreate с Data, AssetsMaximum и
DomainID прошёл, первые два вернулись на объекте, DomainID — нет, он уехал
на связанный share MPTokenIssuance
* LOLoan.PrincipalRequested — поле транзакции LoanSet; реальный займ хранит
сумму как PrincipalOutstanding
* LOCredential.OwnerNode — у объекта IssuerNode/SubjectNode; нулевые hint'ы
сериализуются ("OwnerNode":"0" у Loan), так что отсутствие настоящее
* LONFTokenPage.NFTokenPage, LOAmm.LedgerCurrentIndex, LOAmm.Validated —
последние два принадлежат конверту ответа amm_info (snake_case)
Починено в LOAmm:
* AMMAccount никогда не десериализовался — поле объекта называется Account,
а атрибута не было; имя свойства не меняется, вызовы не ломаются
* конструктор ставил LedgerEntryType.AccountRoot вместо AMM
Добавлены PreviousTxnID/PreviousTxnLgrSeq в LOAmm, LOAmendments,
LODirectoryNode, LOFeeSettings, LONegativeUNL.
Xrpl 10.11.0.0 -> 10.12.0.0. Xrpl.BinaryCodec не менялся и остаётся 10.11.0.0.
Проверено: юнит-тесты 879/879; на nightly-стенде TestILoan/TestIVault/
TestICredential/TestINFToken/TestIAmm — 64/64; сборка решения без ошибок.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
…0.0 в один 10.11.0.0 На NuGet последняя опубликованная версия Xrpl — 10.10.0. Значит 10.10.1.0 (фикс OnConnected), 10.11.0.0 (флаги ledger-объектов, гвард по LedgerFormats.h, DynamicMPT, sfLEVersion) и 10.12.0.0 (гвард полей ledger-объектов и чистка моделей) существуют только в dev и ни один из номеров наружу не уходил. Три раздела CHANGES.md слиты в один 10.11.0.0 от 08/04/2026, версия пакета Xrpl откачена с 10.12.0.0 на 10.11.0.0. Потребитель получит один релиз вместо трёх номеров, из которых два никогда не существовали как артефакты. Xrpl.BinaryCodec уже стоял на 10.11.0.0 — после схлопывания он совпадает с Xrpl ровно, как и задумывалось при выравнивании. AddressCodec и Keypairs остаются на 10.9.0.0: они не менялись. Проверено: сборка решения без ошибок, юнит-тесты 879/879.
Platonenkov
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Xrpl/Models/Ledger/LONegativeUNL.cs (1)
32-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider the short attribute form for consistency with the sibling models.
LOAmendments,LODirectoryNode,LOFeeSettings, andLOAmmall use[JsonPropertyName("...")]. This file uses the fully qualified form. The fully qualified form compiles either way, so this is style only. Ifusing System.Text.Json.Serialization;is already present in this file, shorten both attributes.♻️ Proposed alignment
- [System.Text.Json.Serialization.JsonPropertyName("PreviousTxnID")] + [JsonPropertyName("PreviousTxnID")] public string PreviousTxnID { get; set; } /// <summary> /// The index of the ledger that contains the transaction that most recently modified this object. /// </summary> - [System.Text.Json.Serialization.JsonPropertyName("PreviousTxnLgrSeq")] + [JsonPropertyName("PreviousTxnLgrSeq")] public uint? PreviousTxnLgrSeq { get; set; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Xrpl/Models/Ledger/LONegativeUNL.cs` around lines 32 - 39, Use the imported JsonPropertyName symbol in both attributes on PreviousTxnID and PreviousTxnLgrSeq within the LONegativeUNL model, replacing the fully qualified System.Text.Json.Serialization.JsonPropertyName form while preserving the property names and behavior.Tests/Xrpl.Tests/Models/RippledLedgerEntryFormats.cs (1)
131-132: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider failing on a duplicate ledger-object name instead of overwriting.
Line 131 uses indexer assignment. If two blocks ever declare the same object name, the second silently replaces the first, and that object leaves the conformance table. Line 132 still counts both field sets, so the minimum-count guard at Line 135 does not detect the loss. No duplicate name exists in the current fixture, so this is forward-looking only.
♻️ Proposed guard
- entries[name] = fields; + if (entries.ContainsKey(name)) + { + throw new InvalidOperationException( + $"{name}: declared twice in ledger_entries.macro — the parser would drop one " + + "definition, update it before trusting this test"); + } + + entries.Add(name, fields); fieldCount += fields.Count;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/Xrpl.Tests/Models/RippledLedgerEntryFormats.cs` around lines 131 - 132, Update the ledger-entry aggregation around the entries assignment to detect duplicate object names before inserting into entries, and fail immediately instead of overwriting the existing fields. Keep fieldCount updates only for successfully unique entries so duplicate declarations cannot inflate the count while removing an object from the conformance table.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Tests/Xrpl.Tests/Models/RippledLedgerEntryFormats.cs`:
- Around line 131-132: Update the ledger-entry aggregation around the entries
assignment to detect duplicate object names before inserting into entries, and
fail immediately instead of overwriting the existing fields. Keep fieldCount
updates only for successfully unique entries so duplicate declarations cannot
inflate the count while removing an object from the conformance table.
In `@Xrpl/Models/Ledger/LONegativeUNL.cs`:
- Around line 32-39: Use the imported JsonPropertyName symbol in both attributes
on PreviousTxnID and PreviousTxnLgrSeq within the LONegativeUNL model, replacing
the fully qualified System.Text.Json.Serialization.JsonPropertyName form while
preserving the property names and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1ab48fae-be1b-4882-801d-dec94ebca4c8
📒 Files selected for processing (16)
CHANGES.mdTests/Xrpl.Tests/Fixtures/ledger_entries.macroTests/Xrpl.Tests/Fixtures/ledger_entries.macro.refTests/Xrpl.Tests/Integration/transactions/TestILoan.csTests/Xrpl.Tests/Models/RippledLedgerEntryFormats.csTests/Xrpl.Tests/Models/TestULedgerEntryFieldsConformance.csTests/Xrpl.Tests/Xrpl.Tests.csprojXrpl/Models/Ledger/LOAmendments.csXrpl/Models/Ledger/LOAmm.csXrpl/Models/Ledger/LOCredential.csXrpl/Models/Ledger/LODirectoryNode.csXrpl/Models/Ledger/LOFeeSettings.csXrpl/Models/Ledger/LOLoan.csXrpl/Models/Ledger/LONFTokenPage.csXrpl/Models/Ledger/LONegativeUNL.csXrpl/Models/Ledger/LOVault.cs
💤 Files with no reviewable changes (4)
- Xrpl/Models/Ledger/LOVault.cs
- Xrpl/Models/Ledger/LOCredential.cs
- Xrpl/Models/Ledger/LONFTokenPage.cs
- Xrpl/Models/Ledger/LOLoan.cs
* LONegativeUNL: добавлен using System.Text.Json.Serialization и атрибуты сокращены до [JsonPropertyName] — как в LOAmendments, LODirectoryNode, LOFeeSettings и LOAmm. Полная форма стояла потому, что файл этот namespace не импортировал; теперь импортирует * RippledLedgerEntryFormats: парсер падает на повторно объявленном объекте вместо тихой перезаписи. Индексаторное присваивание позволяло второму объявлению вытеснить первое — объект молча уходил из таблицы конформанса, а fieldCount продолжал расти, так что порог минимального разбора потерю не замечал. Это ровно тот класс тихого отказа, против которого написан остальной парсер Дубликатов имён в текущей фикстуре нет (единственный LEDGER_ENTRY_DUPLICATE — DepositPreauth, и обычного объявления с этим именем не существует), так что гвард forward-looking. Проверен мутацией: дублирование блока Ticket даёт "Ticket: declared twice in ledger_entries.macro"; фикстура после проверки восстановлена и снова байт-в-байт совпадает с пином. Проверено: сборка без ошибок, юнит-тесты 879/879.
|
Оба нитпика проверил и принял — коммит 1. 2. Замечание forward-looking: дубликатов имён в текущей фикстуре нет. Единственный Проверил мутацией, а не только тем, что тесты зелёные: продублировал блок Сборка без ошибок, юнит-тесты 879/879. |
1 participant
Зачем
Третья и последняя поверхность конформанса.
TestUTxFormatConformanceдержит поля транзакций,TestULedgerFlagsConformance(из #71) — флаги ledger-объектов, а поля самих объектов не проверял никто.ledger_entries.macro— единственное место, где протокол объявляет состав объекта:definitions.jsonнесёт коды полей и типы объектов, но не пофайловые списки.Пропущенное поле симптома не даёт — чтение объекта проходит, значение молча теряется. Именно так
LOAccountRootжил безWalletLocator/WalletSizeдо ручного completeness-прохода, и так жеsfLEVersionпришлось ловить уведомлением protocol-watch вместо красного теста.Гвард
Tests/Xrpl.Tests/Fixtures/ledger_entries.macro, вендорен байт-в-байт, пин по sha в.ref. Пин на develop-коммит, а не на тег — в отличие отLedgerFormats.h: по полям модели следуют develop, иsfLEVersionсуществует только после 30.07, так что тег объявил бы его выдумкой SDK.LedgerIndex,LedgerEntryType,Flags,SponsorизLedgerFormats::getCommonFields()) исключены с обеих сторон — ровно как TxFormat-гвард обходится сcommonFields.[JsonIgnore]-свойства (вычисляемые хелперыDataParsed,MPTokenMetadataRow) на провод не попадают и тоже исключены.JsonPropertyNameдаёт обе половины отчёта —Loan.Borrower … missing from LOLoanиLOLoan.BorrowerX … not a field of Loan.Удалены свойства, которых нет в протоколе (breaking)
Без
[Obsolete]— как при удалении инертныхConnectionOptionsв 10.10.0.0. Держать значение они не могли: rippled собирает объект по фиксированномуSOTemplate, поле вне шаблона в него не попадает.Проверено двумя независимыми способами: на живой ноде (nightly, 3.3.0-b1) и кросс-версионно по четырём точкам — 3.2.1 (мейннет), 3.3.0-b1, 3.3.0-rc1 (то, что выйдет) и develop. Нет ни в одной, включая невышедшую.
LOVault.DomainIDVaultCreateсData,AssetsMaximumиDomainIDпрошёл; первые два вернулись на объекте,DomainID— нет, и обнаружился на связанном share-MPTokenIssuance. Ровно как гласят комментарий в макросе (no PermissionedDomainID ever (use MPTIssuance.sfDomainID)) и кодVaultCreate.cpp(.domainId = tx[~sfDomainID])LOLoan.PrincipalRequestedLoanSet; настоящий займ, созданный сPrincipalRequested = 10000000, хранит сумму какPrincipalOutstanding, самого поля у объекта нетLOCredential.OwnerNodeIssuerNode/SubjectNode— он висит в двух директориях. Нулевые hint'ы сериализуются (объект Loan вернул"OwnerNode":"0"), так что отсутствие настоящее, а не опущенный дефолтLONFTokenPage.NFTokenPageNFTokens,PreviousPageMin,NextPageMin,PreviousTxn*LOAmm.LedgerCurrentIndex,LOAmm.Validatedamm_info(ledger_current_index,validated, snake_case), а не объекту;LOAmmдесериализуется только как ledger-объект, уamm_infoсвоя модельAMMInfoPositive control здесь принципиален: без него «поля не было в ответе» означало бы лишь «поле не заполнено» —
SoeDefault/SoeOptionalс пустым значением нода не сериализует вовсе.Починено в
LOAmmAMMAccountникогда не десериализовался: поле объекта называетсяAccount, а атрибута[JsonPropertyName]не было — свойство молча оставалосьnullна каждом прочитанном AMM. Теперь замаплено наAccount; имя свойства не менялось, вызовы не ломаются.LedgerEntryType = LedgerEntryType.AccountRoot— объект AMM представлялся AccountRoot'ом. ТеперьLedgerEntryType.AMM.Добавлены отсутствовавшие поля
PreviousTxnIDиPreviousTxnLgrSeqвLOAmm,LOAmendments,LODirectoryNode,LOFeeSettings,LONegativeUNL— всеSoeOptionalв протоколе. Без них нельзя было прочитать через типизированный API, какая транзакция последней тронула объект.Проверка
TestILoan|TestIVault|TestICredential|TestINFToken|TestIAmmИнтеграцию гонял намеренно по всем затронутым моделям: удаление свойств и добавление
PreviousTxn*меняет десериализацию, и это стоило проверить на живых объектах.Версия
Все накопившиеся в
devизменения выпускаются одним номером —10.11.0.0. На NuGet последняя опубликованнаяXrpl— 10.10.0, поэтому 10.10.1.0 (фиксOnConnected), 10.11.0.0 (флаги ledger-объектов, гвард поLedgerFormats.h, DynamicMPT,sfLEVersion) и 10.12.0.0 (этот PR) существовали только в ветке и наружу не уходили. Три разделаCHANGES.mdслиты в один, версия пакета откачена с 10.12.0.0 до 10.11.0.0 — потребитель получает один релиз вместо трёх номеров, два из которых никогда не были артефактами.Xrpl.BinaryCodecуже стоял на 10.11.0.0, так что после схлопывания совпадает сXrplровно, как и задумывалось при выравнивании.Xrpl.AddressCodecиXrpl.Keypairsне менялись и остаются на 10.9.0.0.Summary by CodeRabbit
New Features
Bug Fixes
Documentation