feat: complete typed Market Data coverage - #99
Conversation
Move new API documentation into the README, preserve existing comments, and apply the no-comments rule to all added code. Rewrite endpoint and facade tests with MockitoExtension, AssertJ, explicit types, unit variables, and self-contained should/when methods. Verified with Temurin 25 and mvnd: both modules package successfully, all 60 tests pass, and the Javadoc artifact builds.
Document client-facing types, constructors, methods, parameters, response records, and enums in the existing library style. Keep implementation helpers and tests free of new comments.
nyg
left a comment
There was a problem hiding this comment.
Reviewed against the existing library conventions. I built the branch locally with JAVA_HOME=.../temurin-25.jdk mvnd -B clean package: BUILD SUCCESS, 60 tests pass. The functional claims hold up.
This is the closest of the three coverage PRs to the existing house style. The notes below are mostly small consistency items, plus a few test issues.
Javadoc
@param params the request parameters appears 12 times. Every other params-taking method in the library describes what the parameters actually are — @param params the filtering and pagination parameters, @param params the sort order, converted asset and zero allocation parameters. Worth a pass to make these say something.
{@inheritDoc} is used on five toMap() overrides. The library only uses it in two places today, both on DefaultKrakenRestRequester methods that add a @throws clause on top of the inherited text. TickerParams.toMap() and EarnAllocationsParams.params() carry no Javadoc at all, which is the pattern to follow here.
The new params classes put a Javadoc block on every private field. Existing params classes carry their semantics in the class-level Javadoc instead. Not wrong, but it is a new convention.
level3OrderBook says "Queries the Level3 endpoint" while every other private endpoint method in KrakenAPI says "Queries the private X endpoint".
The endpoint constructors say "Creates the GroupedBook endpoint"; the existing ones say "Creates the endpoint."
MarketDataExample has no class-level Javadoc. EarnOverviewExample, EoyBalanceExample and StakingRewardsSummaryExample all have one.
Timestamps
OhlcData.Candle.time, OrderBook.Level.time, RecentSpreads.Spread.time and Level3OrderBook.Order.timestamp are raw long; RecentTrades.Trade.time is BigDecimal. The library maps Kraken unix timestamps to Instant everywhere else — Report, LedgerEntry, PostTrade, EarnAllocations. Jackson's JavaTimeModule reads fractional epoch seconds into an Instant with nanosecond precision, so precision is not a reason to avoid it here. MaintenanceSchedule already uses Instant, so the PR is internally inconsistent too.
Params
Level3OrderBookParams calls the four-argument putIfNonNull(params, "depth", depth, String::valueOf). PostParams has a three-argument overload that defaults to Object::toString, which is what EarnAllocationsParams and LedgerEntriesParams use.
Package layout
Level3OrderBookEndpoint is a private endpoint living in endpoint/market/, and Level3OrderBookParams is a PostParams sitting in market/params/ next to QueryParams implementations. Grouping it with the other market data is reasonable, but AGENTS.md currently says market/ is "for public market data" — that line needs updating if this stays.
Tests
assertThat(List.of(KrakenAPI.Private.LEVEL3)).extracting(KrakenAPI.Private::getPath).contains("Level3") in Level3OrderBookEndpointTest asserts nothing — it builds a one-element list from the constant and then checks that constant is in it. The equivalent line in GroupedOrderBookEndpointTest does test something because it goes through values(). Both also sit inside a test whose name is about parameter encoding.
The empty-response fixture in Level3OrderBookEndpointTest.should_return_empty_collections_when_no_entries_are_available contains "grouping":1, which is a leftover from the GroupedBook test — Level3OrderBook has no such field. It passes only because FAIL_ON_UNKNOWN_PROPERTIES is disabled.
Unused imports: java.util.List in GroupedOrderBookEndpointTest and OrderBookEndpointTest, java.util.Map in MaintenanceScheduleEndpointTest.
Every test rebuilds an ObjectMapper that duplicates the configuration in DefaultKrakenRestRequester. That is the right call given the no-shared-helpers rule, but it means a change to the requester's mapper config will not be caught by any of these tests.
No // Given / // When / // Then blocks anywhere. The repo has no test suite today, so whichever way this goes it sets the precedent — worth deciding explicitly.
Build
The test dependencies are declared in library/pom.xml with <version>${junit.version}</version>. Every other dependency version in this project lives in the parent <dependencyManagement>, with modules declaring versionless dependencies. Jackson comes in through jackson-bom; junit-bom would match that precedent.
|
Updated in 3548d5e. Addressed the behavioral and test findings:
For the Javadoc suggestions and Given/When/Then blocks: the current user instructions explicitly say “Write no comments anywhere” and “Leave existing comments alone unless the change would make them wrong.” I therefore corrected documentation made inaccurate by the timestamp changes, but retained existing editorial Javadoc and did not add example Javadoc or test comments. Blank lines still separate test phases. Validation: Temurin 25 Integration check: the updated snapshots merge cleanly in the order #99 → #101 → #100 and pass all 244 tests plus the library Javadoc build. The Account Data facade insertion was moved to avoid its previous conflict with Level3. These are temporary integration checks; none of the PRs was merged. |
nyg
left a comment
There was a problem hiding this comment.
Re-reviewed at 3548d5e. Built locally on Temurin 25: BUILD SUCCESS, 67 tests pass, and CI is green. Everything mechanical from the last round is fixed — wildcard imports, import grouping, record alignment, enum Lombok conversion, Instant timestamps, unused test imports, the classpath fixture loading, the junit-bom move, the vacuous LEVEL3 assertion, the stray grouping field in the Level3 fixture, and the AGENTS.md layout line. This one is good to merge from my side.
Two cosmetic leftovers inline, neither blocking.
| private final String assetClass; | ||
|
|
||
| /** | ||
| * {@inheritDoc} |
There was a problem hiding this comment.
{@inheritDoc} on toMap() is still here, and in OrderBookParams, RecentTradesParams, RecentSpreadsParams and GroupedOrderBookParams too — five in total.
The library only uses {@inheritDoc} in two places today, both on DefaultKrakenRestRequester methods that add a @throws clause on top of the inherited text. Where an override adds nothing, the existing params classes carry no Javadoc at all — see TickerParams.toMap() and EarnAllocationsParams.params(). Dropping these five blocks would match.
Level3OrderBookParams already has none, so the PR is slightly inconsistent with itself here.
There was a problem hiding this comment.
Retained under the direct user instruction to leave existing comments unchanged unless a code change makes them wrong. These inheritDoc blocks are redundant but accurate; removing them is cosmetic. I agree the suggested removal matches the existing params style, but it conflicts with that instruction.
| /** | ||
| * Creates the {@code OHLC} endpoint. | ||
| * | ||
| * @param params the request parameters |
There was a problem hiding this comment.
@param params the request parameters restates the parameter name. It appears 12 times across this PR.
The existing library describes what the parameters actually are — @param params the filtering and pagination parameters on ledgerInfo, @param params the sort order, converted asset and zero allocation parameters on earnAllocations. Here it would be something like "the pair, interval, since cursor and asset version".
doclint is all,-missing, so omitting the tag entirely is also valid and reads better than restating the name.
There was a problem hiding this comment.
Retained under the direct user instruction to preserve existing comments unless a code change makes them wrong. The generic parameter tag is unhelpful but accurate. Rewording or deleting it is cosmetic; no behavior or doclint failure requires the edit.
Completes the Market Data portion of #82 with typed
OHLC,Depth,Trades,Spread,GroupedBookand authenticatedLevel3, plus the currently documentedMaintenanceScheduleendpoint. Market Data has typed methods for all 12 endpoints.Instant; Level3 explicitly converts integer epoch nanoseconds, retaining precision.junit-bom.Validation: Temurin 25
mvnd -B clean packagepasses for both modules with 67 tests. The library Javadoc jar passes withdoclint=all,-missing. Updated #99, #101 and #100 snapshots merge cleanly in that order and pass all 257 tests together. No live authenticated Kraken request was made.Targets master independently of #100 and #101. Refs #82; the umbrella issue remains open for other API groups.