Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AccountBalances V7 #36

Closed
Clauscomputing opened this issue Dec 5, 2023 · 7 comments · Fixed by #38
Closed

AccountBalances V7 #36

Clauscomputing opened this issue Dec 5, 2023 · 7 comments · Fixed by #38

Comments

@Clauscomputing
Copy link
Contributor

Hi,

I just tried to fetch AccountBalances from a bank and got:

unsupported versions [7 4]

A bit of searching in the code showed that only version 5 and 6 are supported for now:

var accountBalanceRequests = map[int]func(account domain.AccountConnection, allAccounts bool) AccountBalanceRequest{

Then checked:

https://github.com/raphaelm/python-fints/blob/eb94706a9ba9f91f69e3e734abe8d4e746ea82b8/fints/segments/saldo.py#L67

and it seems AccountConnectionDataElement changed from "Kontoverbindung, version 2" to "Kontoverbindung international, version 1" according to python-fints. The Gesamtdok_HBCI22.pdf however shows in II 5.3.3 the same structure implemented in go-hbci.

If I'm not mistaken the data structure in V7 also includes IBAN and BIC:

https://github.com/raphaelm/python-fints/blob/eb94706a9ba9f91f69e3e734abe8d4e746ea82b8/fints/formals.py#L476

I then tried to follow the implementation of V7 in:

func SepaAccountTransactionRequestBuilder(versions []int) (func(account domain.InternationalAccountConnection, allAccounts bool) *AccountTransactionRequestSegment, error) {

and got lost. Would AccountBalance also require a SepaAccountBalances?

Any hints are highly appreciated. Thanks!

@mitch000001
Copy link
Owner

Hey @Clauscomputing, sorry for the late response.
I had the time to look into it and yes, as the interface changed to having Sepa compliant account connection/account information the way forward would be to introduce the corresponding Sepa function.

I think it would be best to get rid of the duplicating functions and only retain the sepa ones in the Client struct, as most of the non Sepa data structures (AccountConnection, AccountInformation) can be created using their international counterparts. Also, we could infer the usable segments by looking into the bank parameter data up front.

Those changes would of course break the existing interface to the Client, but I imagine it would be worth it for the sake of simplicity.

@mitch000001
Copy link
Owner

Another look into it and I would maybe just duplicate the non-Sepa versions of the functions into the Sepa ones and deprecate the non-Sepa ones. That way we do not break existing code.

@mitch000001
Copy link
Owner

@Clauscomputing I added an implementation of version 7 and 8 of that segment (HKSAL). As version 7+ uses sepa compliant accountConnections, i.e. account data with IBAN and BIC, there is a new function within the client which sends those sepa compliant requests.
Feel free to give it a try :)

@Clauscomputing
Copy link
Contributor Author

@mitch000001 Thanks a lot! I'll give it a try!

@Clauscomputing
Copy link
Contributor Author

@mitch000001 Not sure if I understand the changes correctly and therefore put together a small repo:
https://github.com/Clauscomputing/go-hbci-test

AccountTransactions stopped working for that bank with the PR. No accounts found.
SepaAccountTransactions and SepaAccountBalances are giving errors.

Do I need additional initialization of the SepaAccount? I tried to follow client_feature_test.go

@mitch000001
Copy link
Owner

@Clauscomputing When implementing those balance changes I saw that the account responses are not implemented properly, i.e. when fetching the accounts there is only one response implemented (I think it's V4) and therefore this request could eventually break due to unsupported versions.
I would add some more information into the error output to see which request builder is issuing the warning/error.
From the error I'd guess that the u marshalling of the mentioned field is not working properly.

The banking command exposes a way of enabling the debug logs, but the method is not accessible outside the project. I could try to expose the log level into the client config so that it's possible to enable debug logging also outside the project.

I will come back to you once I have added those things so you can give it a go.

@mitch000001
Copy link
Owner

@Clauscomputing I pushed a new Commit to PR #38 so if you want to provide more low level details about the request you can now enable debug logging in your code snippets. However, be prepared to get a whole lot of details about the raw requests.
Also, this commit adds some more information to the version errors. I'll try to look into the SecurityID error, but as I could not experience the error myself those debug logs would be great to debug this error.

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 a pull request may close this issue.

2 participants