fix(review): замечания CodeRabbit к релизному PR #68 - #69
Merged
Conversation
Валидные находки: - ValidateCheckCreate требует от InvoiceID 256-битное hex-значение. sfInvoiceID - это Hash256, и в этом же релизе такое же правило добавлено для WalletLocator в AccountSet и SignerListSet; CheckCreate был единственным исключением, где малформед проходил валидацию и падал позже уже внутри кодека. Текст исключения не меняется, так что существующий контракт сохранён - порт 6007 CI-стенда привязан к 127.0.0.1: креды лежат в rippled.cfg открытым текстом, а nightly-стенд и так публиковал его только на loopback - TestIConnectionStates: оба теста на исчерпание реконнектов игнорировали победителя Task.WhenAny, поэтому прогон без терминального события мог пройти по таймауту. Теперь проверяется, что победила именно задача события, и что была хотя бы одна попытка реконнекта - TestIProtocolFieldSets: добавлен ассерт на Expiration у mint-time NFT-оффера - нитпики: общий литерал порога парсинга, общий источник common-полей для двух conformance-тестов, убран лишний Link у fixture Отклонённые находки прокомментированы в PR #68. Коротко: - «поднять версии базовых пакетов до 10.10.0.0» противоречит политике из dc9e3f0 (бампаем только изменившийся пакет), а Base/ в этом релизе не менялся вообще. Первопричина ложного срабатывания - устаревший раздел Release Process в CLAUDE.md, он исправлен здесь же - «keepalive-ping отвергается на порту с кредами» не воспроизводится: роль резолвится per-command, guest-команды проходят. Проверено на живой ноде. У сырого send оставлен комментарий про латентный риск
|
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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Разбор ревью CodeRabbit к релизному PR #68. Десять находок: 4 инлайн, 2 «outside diff», 4 нитпика. Восемь применены, две отклонены с обоснованием, ещё одна оказалась уже выполненной.
Применено
🟠 Порт 6007 CI-стенда — только на loopback. В
docker-compose.batchv11.ymlон и так публиковался как127.0.0.1:6007, а в CI-стенде я оставил6007:6007на все интерфейсы, притом что креды лежат вrippled.cfgоткрытым текстом.🟡
ValidateCheckCreateтребует отInvoiceID256-битное hex-значение.sfInvoiceID— этоHash256, и в этом же релизе ровно такое правило добавлено дляWalletLocatorвAccountSetиSignerListSet;CheckCreateостался единственным местом, где проверялось толькоis string. Малформед проходил валидацию и падал позже уже внутри кодека — с ошибкой кодирования вместоValidationException. Текст исключения оставлен прежним (CheckCreate: invalid InvoiceID), поэтому существующийTestVerify_InValid_InvoiceIDпродолжает описывать тот же контракт. Два новых теста по TDD: сперва падали на длине 63/65, не-hex и пустой строке.🟠
TestIConnectionStates— оба теста на исчерпание реконнектов игнорировали победителяTask.WhenAny. Прогон, в котором терминальное событие не приходило вовсе, продолжался по 30-секундному таймауту и мог пройти, маскируя сломанный реконнект. Теперь проверяется, что победила именно задача события, и что была хотя бы одна попытка реконнекта.🟡
TestIProtocolFieldSets— ассерт наExpiration. Тест выставлял поле у mint-time NFT-оффера, но никогда не проверял, что оно прочиталось обратно.🔵 Нитпики: порог парсинга больше не дублируется литералом (
RippledTransactionFormats.MinimumExpectedTransactionsсталinternal), общий набор common-полей для двух conformance-поверхностей вынесен в один хелпер (RippledTransactionFormats.CommonFields), убран лишнийLinkу vendored fixture.Отклонено
🟡 «Поднять версии базовых пакетов до 10.10.0.0». Противоречит политике репозитория. Коммит
dc9e3f0:Проверено фактически:
git diff --stat origin/release...origin/dev -- Base/даёт пустой вывод — базовые пакеты в этом релизе не менялись вообще, версии корректны как есть.Первопричина ложного срабатывания устранена здесь же: CodeRabbit подписал ту находку
Source: Coding guidelines— он вывел правило изCLAUDE.md, где в разделе Release Process было написано «Update version in all.csprojfiles». Этот текст устарел послеdc9e3f0и теперь исправлен, иначе то же замечание повторялось бы на каждом релизном PR.🟠 «Keepalive-ping отвергается на порту 6007 с
Bad credentials». Наблюдение верное —connection.csшлёт keepalive сырымSendMessageмимоRequestManager, поэтому креды туда не попадают. Но следствие не воспроизводится: проверено на живой ноде — ping без кредов проходит успешно, тогда какledger_acceptна том же соединении отвергается.requestRoleвозвращаетFORBIDтолько для команд, требующих роли ADMIN; guest-команды (ping,server_info) обслуживаются нормально. Никаких «recurring false errors» не возникает.Остаточный риск чисто латентный — если через этот сырой путь когда-нибудь отправят admin-команду, она молча отвалится. Оставлен комментарий у строки.
Уже было сделано
🔵 «Записать провенанс vendored fixture». Файл
Tests/Xrpl.Tests/Fixtures/transactions.macro.refсуществует и содержит repo + SHA + дату. Независимо проверил, что пин верный: содержимоеtransactions.macroбайт-в-байт совпадает сXRPLF/rippledнаfd2cc6dcb308fb811960380eba7bf4309934d05d. Провенанс намеренно лежит соседним файлом, а не комментарием внутри.macro, — иначе копия перестанет быть побайтовой и проверка обычнымdiffсломается.Проверено
dotnet test --filter "TestU"— 867/867 (было 865, +2 новых теста наInvoiceID)dotnet test --filter "TestI&TestCategory!=Live"на standalone-стенде — 219 пройдено, 40 пропущено (amendment-gated), 0 паденийTestIConnectionStates|TestIProtocolFieldSets— 11/11, ни одного пропуска, то есть усиленные ассерты действительно исполнились, а не проскочили как skippeddotnet build XrplCSharp.sln— 0 ошибок