XRPLF / clio

An XRP Ledger API Server
https://xrpl.org
ISC License
56 stars 48 forks source link

Fix missing tx from account_tx #1390

Closed cindyyan317 closed 2 months ago

cindyyan317 commented 2 months ago

Since our selectAccountTxForward is no longer inclusive, we should adjust this place. Fix #1389

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 64.51%. Comparing base (b18d73e) to head (c1f6c2f).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #1390 +/- ## =========================================== + Coverage 64.46% 64.51% +0.04% =========================================== Files 251 251 Lines 10585 10583 -2 Branches 5860 5859 -1 =========================================== + Hits 6824 6828 +4 + Misses 1910 1904 -6 Partials 1851 1851 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

intelliot commented 2 months ago

Ouch. The two most common mistakes in programming: syntax errors, error handling, and off-by-one errors.

Should this be removed as well? or is the logic somehow different for fetchNFTTransactions? https://github.com/XRPLF/clio/blob/c1f6c2f983a9ff8253cef9501f09513a1ac61cbb/src/data/CassandraBackend.hpp#L444-L448

cindyyan317 commented 2 months ago

Ouch. The two most common mistakes in programming: syntax errors, error handling, and off-by-one errors.

Should this be removed as well? or is the logic somehow different for fetchNFTTransactions?

https://github.com/XRPLF/clio/blob/c1f6c2f983a9ff8253cef9501f09513a1ac61cbb/src/data/CassandraBackend.hpp#L444-L448

Thanks for pointing it out. This logic is okay. Because the cql is using ">=". To avoid same transaction being returned, +1 is correct. https://github.com/XRPLF/clio/blob/develop/src/data/cassandra/Schema.hpp#L597 is using ">" , +1 is not necessary.

cindyyan317 commented 2 months ago

Test attached: Result from current s2s2.txt

Result from fixed local cliofixed.txt