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(backend): Class to manage btc pending transactions #2598

Merged
merged 13 commits into from
Oct 1, 2024

Conversation

lmuntaner
Copy link
Contributor

Motivation

We want to store pending btc transactions to disable users from making transactions while there is one that hasn't finished.

In this PR, I introduce the module to manage the pending transactions. It adds functionality to add, read and prune the pending transaction.

This struc will then be added to the heap, (hence that I put it inside a directory heap_state) and used in the backend endpoints.

Changes

  • New class BtcUserPendingTransactions in module heap_state/btc_user_pending_tx_state.
  • Add new module to lib.

Tests

  • Test class functionality.

@lmuntaner lmuntaner marked this pull request as ready for review October 1, 2024 08:56
@lmuntaner lmuntaner requested a review from a team as a code owner October 1, 2024 08:56
@lmuntaner
Copy link
Contributor Author

@bitdivine please review

@lmuntaner lmuntaner marked this pull request as draft October 1, 2024 08:57
@lmuntaner lmuntaner force-pushed the lm-add-btc-pending-transaction-impl branch from e0d35e9 to 8627aae Compare October 1, 2024 12:19
@lmuntaner lmuntaner marked this pull request as ready for review October 1, 2024 12:54
pub struct BtcUserPendingTransactions {
/// Map of `user_principal` to `PendingTransactionsMap`;
pending_transactions_map: HashMap<Principal, PendingTransactionsMap>,
/// Maximum number of transactions stored per principal.
Copy link
Member

Choose a reason for hiding this comment

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

Unsure whether this is the actual maximum or the allowed maximum. I think it's the allowed maximum? Maybe tweaking the text would make this clear. Something like this, maybe?

Suggested change
/// Maximum number of transactions stored per principal.
/// Maximum number of transactions that will be stored per `(principal, address)` tuple.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point!

}
}

/// Returns the pending transactions of a specific principal per address.
Copy link
Member

Choose a reason for hiding this comment

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

Can multiple users share the same Bitcoin address? Not if the Bitcoin address is derived from the principal, I guess, but maybe in future. My concern is that if one user makes a transaction, it may not appear for others sharing the same address. This is not going to happen if we commit to not sharing addresses.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the moment, we don't share addreses.

This would be a problem not only for sharing, but if multiple users can make transactions from the same address. Which at the moment is not supported in CFS either.

But it's something to keep in mind for the future. I'll add a comment.

current_utxos: &[Utxo],
now_ns: u64,
) {
if let Some(address_map) = self.pending_transactions_map.get_mut(&principal) {
Copy link
Member

Choose a reason for hiding this comment

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

Do I see correctly that the number of addresses per principal is unbounded?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

Copy link
Member

@bitdivine bitdivine left a comment

Choose a reason for hiding this comment

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

Approving, with an incoming change to limit the number of accounts.

@lmuntaner lmuntaner force-pushed the lm-add-btc-pending-transaction-impl branch from 216d63c to 70e29b8 Compare October 1, 2024 13:37
@lmuntaner lmuntaner merged commit 9c0d64a into main Oct 1, 2024
16 checks passed
@lmuntaner lmuntaner deleted the lm-add-btc-pending-transaction-impl branch October 1, 2024 13:52
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