Skip to content

feat: complete typed Account Data API - #100

Open
nyg wants to merge 2 commits into
masterfrom
codex/account-data-coverage
Open

feat: complete typed Account Data API#100
nyg wants to merge 2 commits into
masterfrom
codex/account-data-coverage

Conversation

@nyg

@nyg nyg commented Sep 8, 2026

Copy link
Copy Markdown
Owner

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 use Instant, with explicit epoch-nanosecond decoding for amendments. CreditLines returns Optional<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 private RebaseMultiplier type is identical to #101's addition; domain-specific AssetClass types retain their distinct wire vocabularies.

Validation: Temurin 25 mvnd -B clean package passes for both modules with 96 tests. The library Javadoc jar passes with doclint=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.

Support nullable credit results and JSON trade-volume requests while preserving decimal precision and public API documentation.

Refs #82

@nyg nyg left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. AccountBalanceEndpoint.buildURL() appends account_id as a URL query parameter. KrakenCredentials.sign hashes url.getPath() only, so that parameter falls outside the signature. Worth confirming Kraken accepts it.
  2. TradeVolume is sent as application/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.

@nyg

nyg commented Sep 10, 2026

Copy link
Copy Markdown
Owner Author

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 junit-bom. Wire-value enums now use Lombok; removing the handwritten getter also removes its malformed Javadoc. Removed redundant request-enum @JsonProperty annotations.

Order, trade and open-position epoch timestamps now use Instant. OrderAmends also uses Instant, with a package-private nested decoder for its integer epoch nanoseconds. JavaTimeModule alone does not interpret those integers as nanoseconds. CreditLines now returns Optional<CreditLines>, including Optional.empty() for a successful null result. Removed duplicated endpoint parameter fields and reused static JSON mappers. Requester tests now exercise real endpoints through a package-private connection factory, without partial mocks. Test names start with the production class and retain a domain suffix to keep independent coverage suites organized.

The remaining design points:

  • Kept unwrapResponse: changing CreditLines to Optional does not eliminate response-envelope handling. The envelope can still have no result for either a successful null payload or a Kraken error. The endpoint hook distinguishes those cases and avoids endpoint-specific branching in the requester, following the existing ZIP response hook pattern.
  • Kept getContentType and JSON TradeVolume requests. Kraken explicitly documents application/json and structured class-qualified pairs: TradeVolume.
  • Kept Balance's query selector and path-only signature. Kraken places account_id under query parameters in Balance; its authentication specification signs the URI path plus nonce/body hash. A generic private-query abstraction would serve only this endpoint today. These are documentation-backed choices; no live authenticated validation is claimed.
  • Kept numeric nonce encoding. Kraken requires an increasing unsigned integer; a nonnumeric custom nonce is invalid regardless of the Java interface returning String. Added a rejection test.
  • TradeVolumeParams.params() also supports the inherited form-encoding API. The reported overwrite cannot happen: providing both pair formats throws before either write, and putIfNonNull skips absent pairs. Added tests for both formats and their mutual exclusion. The JSON endpoint and form path each validate their entry point.
  • Shared RebaseMultiplier between this PR and feat: complete typed Funding API #101 in endpoint.priv. Kept AssetClass types separate: Account Data accepts pair classes such as forex and equity_pair, whereas Funding accepts asset classes such as currency and tokenized_asset; sharing them would allow inappropriate values. Response enums also have a different role.
  • Retained UNKNOWN because the current supplied instructions explicitly require it on every new enum. It is a fallback constant, not a documented Kraken request option; this does not imply server support for unknown.

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 mvnd -B clean package passes, 96 tests; the library Javadoc jar passes with doclint=all,-missing.

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 nyg left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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() {

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is unreachable. TradeVolumeEndpoint overrides encodedParamsWith, so PostParams.encoded() never runs for this endpoint and params() is never called.

Three consequences:

  1. The IllegalArgumentException on line 54 duplicates the identical check in TradeVolumeEndpoint.encodedParamsWith. Only the endpoint's copy can ever fire.
  2. There is a latent bug in the dead path: line 59 does params.put("pair", ...) for the class-qualified form, then line 65 does putIfNonNull(params, "pair", pairs, ...), which would overwrite it. Guarded today only by the exception above.
  3. OBJECT_MAPPER and the JsonProcessingException handling 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));

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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";

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant