feat: complete typed Funding API - #101
Conversation
Preserve pagination, decimal amounts, unlimited deposit limits, and unsuccessful cancellation results across the ten legacy Funding operations. Refs #82
nyg
left a comment
There was a problem hiding this comment.
Reviewed against the existing library conventions. I did not build this branch locally, but #99 and #100 both build clean on Temurin 25 and this one shares their test setup.
DepositLimit with the package-private DepositLimitDeserializer is the best-fitting new code across the three coverage PRs — it matches the RecordMappingStrategy precedent in AGENTS.md for keeping implementation helpers package-private, and the compact-constructor invariant is a nice touch. Most of the notes below are shared with #100.
Formatting
Record component alignment. The nine multi-line records here (Deposit, DepositAddress, DepositMethod, DepositStatus, Withdrawal, WithdrawalAddress, WithdrawalInfo, WithdrawalMethod, WithdrawalStatus) use a flat 8-space continuation. Every multi-line record in the library aligns its components under the opening paren — Ticker, Report, PostTrade, PreTrade, SystemStatus, EarnStrategies, EarnAllocations, with indents from 21 to 34 spaces.
Wildcard imports. 20 in this branch (dev.andstuff.kraken.api.endpoint.funding.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.*. The main sources and #99's tests put static imports first, then java, com, then dev and lombok as one group.
Blank line between dev.andstuff and lombok imports. Added in 10 new files. The repository has 22 files where those imports are adjacent and none with a blank line between them.
Broken Javadoc indentation in all four new enums (AssetClass, DestinationWallet, RebaseMultiplier, SourceWallet), on the getValue() block:
/**
* Returns the value sent to Kraken.
*
* @return the API value
*/Enums
Same points as #100. The new enums hand-roll a value field, constructor and getter; the library uses Lombok for that (KrakenAPI.Public and KrakenAPI.Private are @Getter @RequiredArgsConstructor), and the existing standalone enums (AssetClass, ReportFormat, RemovalType, LockType) are bare constant lists. @JsonProperty on each constant is redundant given ACCEPT_CASE_INSENSITIVE_ENUMS.
UNKNOWN("unknown") with @JsonValue on request-side enums means a caller can send aclass=unknown or rebase_multiplier=unknown to Kraken. The @JsonEnumDefaultValue UNKNOWN rule in AGENTS.md is about forward-compatible deserialization of responses; the existing request enums have no UNKNOWN.
funding/params/AssetClass is the fourth AssetClass enum in the library, after earn/response, subaccount/params and the one #100 adds in account/params, and funding/params/RebaseMultiplier duplicates #100's. Worth deciding whether these should be shared before all three land.
Javadoc
@param params the request parameters appears 10 times; the library describes what the parameters actually are.
Several params fields have Javadoc that restates the field name — Asset being withdrawn., Amount to be withdrawn., The rebase multiplier for {@code Withdraw}. Also, the new params classes put a block on every private field, which existing params classes do not do; they carry their semantics in the class-level Javadoc.
Timestamps
Deposit.time and Withdrawal.time are Long. The library maps Kraken unix timestamps to Instant — Report, LedgerEntry, PostTrade, EarnAllocations. JavaTimeModule handles fractional epoch seconds with nanosecond precision, so precision is not a reason to keep raw longs.
Params
WithdrawParams.params() calls putIfNonNull for asset, key and amount, all of which are @NonNull and therefore cannot be null by the time params() runs. #99 uses params.put("pair", pair) directly for its required field. Same pattern in the other funding params classes.
Tests
FundingRestRequesterTest is a second test class for DefaultKrakenRestRequester (#100 adds AccountRestRequesterTest for the same production class). FundingKrakenAPITest is likewise named after the domain slice rather than the class under test.
Worth a sweep for unused imports — #100's test files carry a copy-pasted header block with around 50 unused imports and these share the same shape.
No // Given / // When / // Then blocks. Worth settling once, since these PRs 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 that precedent.
Merge order
#99, #100 and this PR make identical edits to pom.xml, library/pom.xml and AGENTS.md, so those conflicts are trivial; the README sections will need real merging. This one is in better shape than #100, so #99 then this then #100 seems like the smoothest order.
|
Updated in 933808b. Addressed the formatting and test findings: aligned record components, expanded wildcard imports, removed unused imports, normalized import grouping, used classpath fixtures, and centralized test versions in the parent with Deposit and withdrawal timestamps now use Shared Retained 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 933808b. Built locally on Temurin 25: BUILD SUCCESS, 81 tests pass, CI green. Merged with #99 and #100 onto master in a scratch worktree — no conflicts, 244 tests pass together. RebaseMultiplier moved to endpoint/priv/ in both this PR and #100 with byte-identical content, so that merges cleanly rather than duplicating.
Everything from the last round is fixed: record alignment, wildcard imports, import grouping, the enum Javadoc indentation, enum Lombok conversion, Instant timestamps, the redundant putIfNonNull calls on @nonnull fields in WithdrawParams, unused test imports, and the junit-bom move. DepositLimit with the package-private DepositLimitDeserializer is still the nicest new code across the three PRs.
Two minor leftovers inline, neither blocking.
| ``` | ||
|
|
||
| There are no tests in this project. CI runs `mvn clean package` on PRs targeting `master`. | ||
| The library's JUnit 5 tests use JSON fixtures and Mockito mocks of `KrakenRestRequester`, without network access or API keys. Run them with `mvn -pl library test`. CI runs `mvn clean package` on PRs targeting `master`. |
There was a problem hiding this comment.
Anchoring here since it is the line this PR touches in AGENTS.md.
This PR adds endpoint/funding/ but the layout paragraph just below (line 22) is not updated for it. It still enumerates only market/, account/, subaccount/, transparency/ and earn/.
#99 updated that same sentence to cover the authenticated Level3 endpoint living under market/, so the two PRs touch it and will need a small manual merge anyway. Worth adding funding/ for deposits and withdrawals while you are in there.
There was a problem hiding this comment.
Fixed in acc0c06: the Layout section now documents funding/ as the private Funding (Legacy) domain for deposits, withdrawals and wallet transfers. It is a separate paragraph, so the Level3 wording change in #99 still merges cleanly. Verified all three updated snapshots together: 257 tests and the Javadoc build pass.
| /** | ||
| * Creates the {@code Withdraw} endpoint. | ||
| * | ||
| * @param params the request parameters |
There was a problem hiding this comment.
@param params the request parameters restates the parameter name. It appears 10 times in this PR.
The existing library describes what the parameters are — @param params the filtering and pagination parameters on ledgerInfo, @param params the asset, amount and accounts of the transfer on accountTransfer. Here it would be something like "the asset, key, amount and optional fee cap of the withdrawal".
doclint is all,-missing, so dropping the tag is valid too 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 tag is accurate; replacing or deleting it is cosmetic. The requested Funding package documentation is handled separately in AGENTS.md.
Completes the ten Funding operations from #82 with typed endpoints, parameter builders, response records and KrakenAPI methods: DepositMethods, DepositAddresses, DepositStatus, WithdrawMethods, WithdrawAddresses, WithdrawInfo, Withdraw, WithdrawStatus, WithdrawCancel and WalletTransfer.
Status responses accept both unpaginated transaction arrays and paginated objects with cursors. Deposit limits distinguish finite amounts, unlimited values and missing values. Monetary fields use
BigDecimal; deposit and withdrawal timestamps useInstant. Requests support asset classes, rebase options, Lightning invoice amounts, address confirmation, maximum withdrawal fees and exact wallet names. Cancellation preserves a valid false result.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 #100's addition.Validation: Temurin 25
mvnd -B clean packagepasses for both modules with 81 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. Tests make no network calls and perform no withdrawals or transfers.Targets master independently of #99 and #100. Covers the
/0/privateFunding group from #82, now called Funding (Legacy); Funding (Beta) is separate. Refs #82; the umbrella issue remains open.