Skip to content

Refactor display unit#293

Closed
Sosthene00 wants to merge 2 commits intodevfrom
refactor_display_unit
Closed

Refactor display unit#293
Sosthene00 wants to merge 2 commits intodevfrom
refactor_display_unit

Conversation

@Sosthene00
Copy link
Collaborator

When discussing #288 it appeared that we are using the rust library for FiatCurrency and bitcoin amount formatting. I think it's improper, doesn't respect separation of concerns, and make the changes proposed in #288 harder to implement.

This PR:

  • removes FiatCurrency from the rust library, and recreate it in lib/models
  • removes ApiAmount, which was just a wrapper around rust-bitcoin Amount type
  • replaces all occurences of ApiAmount in the rust library with u64
  • defines a type alias BtcAmount for BigInt in dart, and uses type extension to implement proper display for amounts

The change for FiatCurrency was extremely simple, and I think that piece of code makes so much more sense in dart than in rust. ApiAmount was a little harder to remove but in the end I think it's better this way too, and it should make the change proposed in #288 really simple to implement.

@Sosthene00 Sosthene00 force-pushed the refactor_display_unit branch from 5a4ac52 to 3948b27 Compare February 16, 2026 09:37
@cygnet3
Copy link
Owner

cygnet3 commented Feb 16, 2026

  • removes FiatCurrency from the rust library, and recreate it in lib/models

I definitely agree with this.

  • removes ApiAmount, which was just a wrapper around rust-bitcoin Amount type

  • replaces all occurences of ApiAmount in the rust library with u64

  • defines a type alias BtcAmount for BigInt in dart, and uses type extension to implement proper display for amounts

This I don't entirely agree with. I prefer to use as little primitive types as possible, so I'd rather we didn't just pass a u64 to the rust side. But yes, all the display-related code shouldn't be in the rust part.

I prefer an approach where we keep the type definition on the rust side (whether that's ApiAmount or BtcAmount), and keep using that in the rust api where we deal with amounts. To implement display logic, we instead define an extension on top of this rust struct on the flutter side, in the same way as we did for Bip353Address. So something like

extension Display on ApiAmount {
  String asBtc() { ... }

  String asSatoshis() { ... }
}

@Sosthene00
Copy link
Collaborator Author

Yeah I made some back and forth around ApiAmount, and while I agree not to have primitive types is usually better I was also judging that ApiAmount was adding a lot of code both rust and dart sides for very little benefit compared to an extension of BigInt.

As far as I understand the main raison d'être of Amount is to not getting confused between sats and btc amounts, and to implement overflow checks on operation of those amounts. Maybe some trade-off would be to expose more rust methods that would basically handle all operations on Amounts, i.e. when calling displayBtc we call a rust method that converts the u64 into Amount, then calls .to_btc() and returns the f32 value. We should also examine everywhere we add and substract amounts and expose ad hoc methods in rust to handle that. That's possible but maybe overengineered at this point?

@cygnet3 cygnet3 force-pushed the refactor_display_unit branch from 3948b27 to 5c6a926 Compare February 18, 2026 00:46
@cygnet3
Copy link
Owner

cygnet3 commented Feb 18, 2026

(rebased on top of dev)

@Sosthene00
Copy link
Collaborator Author

Moved the fiat currency commit into a new PR #306 as I think that ApiAmount deserves its own

@Sosthene00
Copy link
Collaborator Author

Superseded by #307, closing

@Sosthene00 Sosthene00 closed this Feb 23, 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