Skip to content

hotfix: Funding fixes 6.11.0 - #2272

Merged
ethicnology merged 4 commits into
hotfix/6.11.1from
funding-fixes-6110
Jun 9, 2026
Merged

hotfix: Funding fixes 6.11.0#2272
ethicnology merged 4 commits into
hotfix/6.11.1from
funding-fixes-6110

Conversation

@i5hi

@i5hi i5hi commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator

Fixes pending issues in funding flow

@i5hi i5hi changed the title Funding fixes 6110 hotfix: Funding fixes 6110 Jun 9, 2026
@ethicnology
ethicnology changed the base branch from main to hotfix/6.11.1 June 9, 2026 11:56
@ethicnology

Copy link
Copy Markdown
Member

What is your opinion @claude ?

@ethicnology ethicnology mentioned this pull request Jun 9, 2026
@claude

claude Bot commented Jun 9, 2026

Copy link
Copy Markdown

Claude finished @ethicnology's task in 1m 36s —— View job


My opinion on this PR

I read the diff against origin/hotfix/6.11.1 (the funding-error files, the bloc, the new copy/error widgets, and the .arb files). Overall this is a solid, well-structured hotfix — it does the right thing by propagating the backend error code + messageData all the way to presentation and centralizing the code→copy mapping. A few things are worth a look before merge.

What's good 👍

  • Clean layering. code / message / messageData now flow from the gateway → application error → FundExchangeApiErrorFundExchangeApiErrorCopy. Making displayTitle/displayMessage abstract on the sealed FundExchangePresentationError forces every subclass to implement them — good compile-time safety.
  • Localization is consistent. All locales carry the same keys, and the parameterized strings ({missingFields}, {error}, {iban}) keep the placeholders intact in translations, so gen-l10n won't break.
  • Robustness improvements in bullbitcoin_api_funding_gateway.dart: null-safe element (?? {}), num→String coercion before fromJson, and rethrowing typed errors instead of leaking raw parse exceptions.

Things I'd flag

  1. Redundant empty-list throw. The gateway now throws FetchInstitutionsFailed.emptyList() when the result is empty, and the bloc re-checks if (result.institutions.isEmpty) at fund_exchange_bloc.dart:86. Since the gateway already throws, the bloc branch is effectively dead for the API-empty case. Harmless defense-in-depth, but worth a comment so a future reader doesn't think the bloc check is load-bearing.

  2. numTelefono key-guessing is fragile. The merged['numTelefono'] ??= merged['NUM TELEFONO'] ?? merged['NUM_TELEFONO'] ?? merged['PHONE NUMBER'] ?? ... fallback hard-codes several spellings. It works, but it's brittle against backend key changes — a short // backend sends this field under inconsistent keys comment would help, and ideally the backend contract gets pinned down later.

  3. log.severelog.warning downgrade. Since these are now user-handled errors, warning is defensible — but the generic parse-failure path now logs at warning and throws FetchFundingDetailsFailed(message: 'Could not parse details') with no code. If parse failures were something you wanted alerting on, you've lowered their visibility. Confirm that's intentional.

  4. Dead-ish field. LoadInstitutionsFailed gained a message field, but the only producer (InvalidInstitution() mapping) never sets it — so message is always ''. Minor; either wire it up or drop it.

  5. Scope creep (minor). backup_card.dart (SpacerExpanded/Gap overflow fix) is unrelated to funding. Fine to ride along in a hotfix, but it's not described by the PR title/body — worth a one-line mention in the description.

Verdict

No blocking bugs that I can see — the error-mapping logic is correct and the localization is complete. Items 1–4 are polish/observability nits, not correctness issues. I'd be comfortable merging after a quick confirmation on the logging downgrade (#3) and a note about the unrelated backup_card change (#5).

One thing I could not do here: run flutter analyze / the test suite to confirm it compiles cleanly (no build env available in this run). I'd recommend CI green on analyze before merge given the new abstract methods + l10n additions.
· funding-fixes-6110

@ethicnology ethicnology changed the title hotfix: Funding fixes 6110 hotfix: Funding fixes 6.11.0 Jun 9, 2026
@ethicnology
ethicnology merged commit 3f62d39 into hotfix/6.11.1 Jun 9, 2026
2 checks passed
@claude claude Bot mentioned this pull request Jul 5, 2026
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.

2 participants