feat: complete typed Account Data API - #100
Conversation
Support nullable credit results and JSON trade-volume requests while preserving decimal precision and public API documentation. Refs #82
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, 92 tests pass, so the functional claims hold up. The endpoint coverage itself looks right.
The main concern is that this PR drifts from the existing style in several mechanical ways, and it also drifts from #99, so merging both as-is would leave two different conventions in the same library. There are also a few design points on the core abstractions worth a second look.
Formatting
Record component alignment. Every multi-line record in the library aligns its components under the opening paren — Ticker, Report, PostTrade, SystemStatus, PreTrade, EarnStrategies, EarnAllocations, with indents from 21 to 34 spaces. The six multi-line records here (Order, ClosedOrders, ExtendedBalance, OrderAmends, TradeVolume, TradesHistory, WalletAccounts) use a flat 8-space continuation instead.
Wildcard imports. There are 28 of them in this branch (dev.andstuff.kraken.api.endpoint.account.params.*, ...response.*, org.mockito.ArgumentMatchers.*, org.mockito.Mockito.*). The repository has zero, and #99 adds zero.
Import block order. The test classes put static imports last and org.* after dev.*. Both the main sources and #99's tests put static imports first, then java, then com, then dev and lombok as one group.
Blank line between dev.andstuff and lombok imports. Added in 14 new files. The repository has 22 files where those two are adjacent and none with a blank line between them. It is also inserted into PrivateEndpoint.java and DefaultKrakenRestRequester.java, which is unrelated diff noise in files this PR otherwise barely touches.
Broken Javadoc indentation in all five new enums (AssetClass, CloseTime, Consolidation, RebaseMultiplier, TradeType), on the getValue() block:
/**
* Returns the value sent to Kraken.
*
* @return the API value
*/Enums
The new enums hand-roll a value field, a constructor and a getter. Where the library needs a wire value on an enum it uses Lombok — see KrakenAPI.Public and KrakenAPI.Private, both @Getter @RequiredArgsConstructor over private final String path. The existing standalone enums (AssetClass, ReportFormat, RemovalType, LockType) are bare constant lists.
@JsonProperty on each constant is redundant: the mapper is built with ACCEPT_CASE_INSENSITIVE_ENUMS, which is exactly why earn/response/AssetClass gets away with bare CURRENCY, TOKENIZED_ASSET.
UNKNOWN("unknown") combined with @JsonValue means these are also serialized on the way out, so a caller can send rebase_multiplier=unknown or aclass=unknown to Kraken. The AGENTS.md rule about @JsonEnumDefaultValue UNKNOWN is about forward-compatible deserialization of responses; the existing request-side enums (ReportFormat, RemovalType, subaccount/params/AssetClass) have no UNKNOWN.
account/params/AssetClass is a third copy of an AssetClass enum, after earn/response and subaccount/params. #101 adds a fourth in funding/params, and a second RebaseMultiplier. Worth deciding whether these should be shared before all three merge.
Javadoc
@param params the request parameters appears 14 times; the library describes what the parameters are.
Order has @param status the status, @param cost the cost, @param fee the fee, @param price the price, @param trigger the trigger, @param reason the reason, @param miscellaneous the miscellaneous. doclint is set to all,-missing, so omitting these entirely is legal and reads better than restating the field name. AGENTS.md asks for @param/@return "where they add information".
The order returned by {@code OpenOrders, ClosedOrders and QueryOrders}. wraps a prose phrase including a comma and "and" in {@code}. Same in Order.Status. The library uses {@code} around individual identifiers.
Timestamps
Order.openTime, startTime, expireTime, closeTime, AccountTrade.time and TradesHistory times are BigDecimal; OrderAmends timestamps are Long. The library maps Kraken unix timestamps to Instant throughout — Report, LedgerEntry, PostTrade, EarnAllocations. JavaTimeModule reads fractional epoch seconds into an Instant with nanosecond precision, so precision is not a reason to keep the raw types.
Design
New public API on the core types. Endpoint.unwrapResponse and PrivateEndpoint.getContentType are both published API (per AGENTS.md, everything public in library is), each added to serve one endpoint. unwrapResponse exists so CreditLines can return null; the library reaches for Optional in that situation elsewhere (AssetPairs.findBy), and Optional<CreditLines> would avoid a null-returning public method.
TradeVolumeParams.params() is dead code. TradeVolumeEndpoint overrides encodedParamsWith, so params() never runs. It also duplicates the "pairs or pairsWithClass, not both" validation that the endpoint already does, and it contains a latent bug: it does params.put("pair", json) and then putIfNonNull(params, "pair", pairs, ...), so the second would silently overwrite the first.
TradeVolumeEndpoint builds a fresh JsonMapper on every call, in two places. DefaultKrakenRestRequester keeps a single static mapper.
new BigInteger(nonce) throws NumberFormatException for any custom KrakenNonceGenerator that returns a non-numeric string — the interface's contract is just String.
AccountBalanceEndpoint and TradeVolumeEndpoint each keep their own params field, duplicating PrivateEndpoint.postParams, which already has a @Getter. No other endpoint in the library does this.
Two behaviors are unverified. The description notes no live authenticated request was made, and neither of these can be confirmed offline:
AccountBalanceEndpoint.buildURL()appendsaccount_idas a URL query parameter.KrakenCredentials.signhashesurl.getPath()only, so that parameter falls outside the signature. Worth confirming Kraken accepts it.TradeVolumeis sent asapplication/json. Worth confirming against a real call before this ships.
AccountBalanceEndpoint.buildURL() also builds its URL by string-concatenating onto super.buildURL(). If private endpoints need query parameters generally, that probably belongs in PrivateEndpoint.
Tests
Unused imports, roughly 50 across the test files. java.math.BigDecimal, java.util.List, KrakenException and assertThatThrownBy are imported and unused in most of AccountBalanceEndpointTest, ClosedOrdersEndpointTest, OpenOrdersEndpointTest, OpenPositionsEndpointTest, OrderAmendsEndpointTest, ApiKeyInfoEndpointTest, TradeBalanceEndpointTest, TradesHistoryEndpointTest, WalletAccountsEndpointTest, ExtendedBalanceEndpointTest and others. TradeVolumeEndpointTest additionally has URLDecoder, StandardCharsets, Arrays, Map and Collectors unused. Looks like a copy-pasted header block.
Fixtures are read from the filesystem: Files.readString(Path.of("src/test/resources/account/Balance.json")). That depends on the surefire working directory. #99 uses getClass().getResourceAsStream("/market/..."), which is the portable form.
Mocking final record types. TradeVolume, CreditLines, ApiKeyInfo, OpenOrders, ClosedOrders, Order, TradesHistory and friends are records, so mocking them forces Mockito's inline mock maker. The build already prints Mockito is currently self-attaching to enable the inline-mock-maker. This will no longer work in future releases of the JDK. Constructing the records directly is simpler and does not carry that warning.
thenCallRealMethod() is used four times on @Mock endpoints in AccountRestRequesterTest, which is partial mocking.
Test class naming. AccountKrakenAPITest and AccountRestRequesterTest are named after the domain slice rather than the class under test. AccountRestRequesterTest tests DefaultKrakenRestRequester, and #101 adds a FundingRestRequesterTest for the same class, so that production class ends up with two or three test classes.
No // Given / // When / // Then blocks. Worth settling one way or the other since this and #99 establish the project's first test suite.
Build
Test dependencies are declared in library/pom.xml with explicit <version>. Every other version in this project lives in the parent <dependencyManagement>, and Jackson comes in via jackson-bom; junit-bom would match.
Merge order
#99, #101 and this PR make identical edits to pom.xml, library/pom.xml and AGENTS.md, so those conflicts are trivial, but the README sections will need real merging. Since this PR is the only one touching the core abstractions, it is probably worth landing last.
|
Updated in 7d6ec38. Addressed the consistency findings: aligned record components, expanded wildcard imports, removed unused imports, normalized import grouping, loaded fixtures from the classpath, replaced response-record mocks with local values, and moved test versions into parent dependency management with Order, trade and open-position epoch timestamps now use The remaining design points:
The current instructions also prohibit adding comments and require retaining existing comments unless a change makes them wrong. That takes precedence over the editorial Javadoc and Given/When/Then suggestions; documentation made inaccurate by the type changes was corrected. 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 7d6ec38. Built locally on Temurin 25: BUILD SUCCESS, 96 tests pass, CI green. I also merged all three coverage branches onto master in a scratch worktree — no conflicts, 244 tests pass together, and RebaseMultiplier landed at the same path in both this PR and #101 with identical content, so that merges cleanly.
Most of the last round is fixed: record alignment, wildcard imports, import grouping, enum Lombok conversion, Instant timestamps, the static OBJECT_MAPPER, CreditLines returning Optional instead of null, the duplicated params fields now going through getPostParams(), ~50 unused test imports, classpath fixture loading, thenCallRealMethod, and the junit-bom move.
Four things left inline. Only the first is a real defect; the two signing/content-type questions just need one live call to confirm.
| private final RebaseMultiplier rebaseMultiplier; | ||
|
|
||
| @Override | ||
| protected Map<String, String> params() { |
There was a problem hiding this comment.
This method is unreachable. TradeVolumeEndpoint overrides encodedParamsWith, so PostParams.encoded() never runs for this endpoint and params() is never called.
Three consequences:
- The
IllegalArgumentExceptionon line 54 duplicates the identical check inTradeVolumeEndpoint.encodedParamsWith. Only the endpoint's copy can ever fire. - There is a latent bug in the dead path: line 59 does
params.put("pair", ...)for the class-qualified form, then line 65 doesputIfNonNull(params, "pair", pairs, ...), which would overwrite it. Guarded today only by the exception above. OBJECT_MAPPERand theJsonProcessingExceptionhandling in this class exist solely for that dead branch.
Simplest fix is to delete params() and the mapper here and let the endpoint own the encoding, or go the other way and drop the endpoint override. Either way the validation should live in exactly one place.
| throw new IllegalArgumentException("Specify pairs or pairsWithClass, not both"); | ||
| } | ||
| Map<String, Object> body = new LinkedHashMap<>(); | ||
| body.put("nonce", new BigInteger(nonce)); |
There was a problem hiding this comment.
new BigInteger(nonce) throws NumberFormatException on any non-numeric nonce.
KrakenNonceGenerator.generate() returns String with no documented numeric constraint, and KrakenAPI lets callers supply their own generator. EpochBasedNonceGenerator happens to be numeric, so this only bites a custom implementation — and it fails at request time with an exception that does not explain the cause.
If Kraken requires the nonce as a JSON number here, worth saying so in the Javadoc and either tightening the KrakenNonceGenerator contract or wrapping this in a clearer IllegalStateException.
| */ | ||
| @Override | ||
| public String getContentType() { | ||
| return "application/json"; |
There was a problem hiding this comment.
This is the one endpoint in the library that sends application/json rather than form encoding, and the PR description notes no live authenticated request was made.
Worth one real call to confirm Kraken accepts a JSON body on TradeVolume before this ships, since a mistake here is only visible at runtime against the real API. The unit tests mock the requester, so they cannot catch it.
| return super.buildURL(); | ||
| } | ||
| try { | ||
| return URI.create(super.buildURL() + "?account_id=" + URLEncoder.encode(params.getAccountId(), StandardCharsets.UTF_8)).toURL(); |
There was a problem hiding this comment.
account_id goes into the URL query, but KrakenCredentials.sign builds its HMAC from url.getPath() only — the query string is not part of the signature.
That may well be what Kraken expects, but it is the kind of thing that fails with EAPI:Invalid signature rather than anything descriptive. Same caveat as the JSON body above: unverified against a live call, and unit tests with a mocked requester will not catch it.
Separately, if private endpoints need query parameters as a general capability, this probably belongs in PrivateEndpoint rather than string-concatenating onto super.buildURL() in one subclass.
| /** | ||
| * Creates the {@code OpenPositions} endpoint. | ||
| * | ||
| * @param params the request parameters |
There was a problem hiding this comment.
@param params the request parameters restates the parameter name. It appears 14 times in this PR.
The existing library describes what the parameters are — @param params the filtering and pagination parameters on ledgerInfo, @param params the sort order, converted asset and zero allocation parameters on earnAllocations.
doclint is all,-missing, so dropping the tag is valid too and reads better than restating the name.
Completes the missing typed Account Data operations from #82: Balance, BalanceEx, CreditLines, TradeBalance, OpenOrders, ClosedOrders, QueryOrders, OrderAmends, TradesHistory, QueryTrades, OpenPositions, TradeVolume, GetApiKeyInfo and ListWalletAccounts. Existing ledger and report methods remain available.
Balances and quantities use
BigDecimal; numeric order and trade timestamps useInstant, with explicit epoch-nanosecond decoding for amendments. CreditLines returnsOptional<CreditLines>, including an empty optional for a successful null result. Balance places wallet selection in the URL query. TradeVolume supports class-qualified pair arrays and complete fee schedules through its documented JSON request body. The requester sends the matching content type and delegates response-envelope handling to the endpoint.Response-record tests use real values, fixtures load from the classpath, and requester tests exercise real endpoints through a package-private connection factory. Test dependency versions are managed in the parent with
junit-bom. The shared privateRebaseMultipliertype is identical to #101's addition; domain-specific AssetClass types retain their distinct wire vocabularies.Validation: Temurin 25
mvnd -B clean packagepasses for both modules with 96 tests. The library Javadoc jar passes withdoclint=all,-missing. Updated #99, #101 and #100 snapshots merge cleanly in that order and pass all 244 tests together. Tests use mocks, without API keys or live Kraken calls.Targets master independently of #99 and #101. Refs #82; the umbrella issue remains open for other API groups.