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

feat: Add wallet history to webapp #1860

Merged
merged 3 commits into from
Jan 22, 2024
Merged

feat: Add wallet history to webapp #1860

merged 3 commits into from
Jan 22, 2024

Conversation

holzeis
Copy link
Contributor

@holzeis holzeis commented Jan 21, 2024

Screenshot 2024-01-21 at 20 12 06

@holzeis holzeis requested review from bonomat and luckysori January 21, 2024 19:15
@holzeis holzeis self-assigned this Jan 21, 2024
@holzeis holzeis force-pushed the feat/add-wallet-history branch from 384d3d3 to b93f9d7 Compare January 21, 2024 19:21
Copy link
Contributor

@luckysori luckysori left a comment

Choose a reason for hiding this comment

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

LGTM.

@@ -8,6 +8,7 @@ use axum::Json;
use native::api;
use native::api::Fee;
use native::api::SendPayment;
use native::api::WalletHistoryItemType;
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 I thought the idea was that we would reuse everything in native except for the stuff in the api modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fear the models are only on the api layer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right! I think it might pay off to fix that, because we probably don't want to change the web app models when we are working on the mobile app.

For now we could just duplicate a couple of times.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not so sure about that. In the past we've copied the models extensively for the different modules (common, database, api, etc) and it was very cumbersome to write all this boilerplate code to copy data from one model to the other (which was mostly exactly the same).

For now I suggest we keep it as is and revisit if we run into any issues with it. Note, the api models used between webapp ui and webapp backend are decoupled from the native crate.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would at least be sensible to move the shared code to a place that is not explicitly designed to be consumed by flutter_rust_bridge though.

webapp/src/api.rs Show resolved Hide resolved
@holzeis holzeis added this pull request to the merge queue Jan 22, 2024
Merged via the queue into main with commit 72b86fc Jan 22, 2024
9 checks passed
@holzeis holzeis deleted the feat/add-wallet-history branch January 22, 2024 08:09
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