-
Notifications
You must be signed in to change notification settings - Fork 23
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: Automatically rollover in rollover weekend #1287
Merged
Merged
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
d4b14ca
chore: Set position expiry only in one place
holzeis cdd708e
feat: Calculate expiry timestamp to Sunday 3pm
holzeis 48e857a
feat: Automatically rollover if user opens app during rollover weekend.
holzeis 63932dc
fix: Only update open or rollover positions
holzeis f3fa8db
chore: Separate components into separate modules
holzeis 322ab39
fix: Do not rollover an expired position
holzeis File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,84 @@ | ||
use anyhow::Result; | ||
use bitcoin::secp256k1::PublicKey; | ||
use futures::future::RemoteHandle; | ||
use futures::FutureExt; | ||
use orderbook_commons::Message; | ||
use std::collections::HashMap; | ||
use std::sync::Arc; | ||
use std::sync::RwLock; | ||
use tokio::sync::broadcast; | ||
use tokio::sync::mpsc; | ||
|
||
/// This value is arbitrarily set to 100 and defines the message accepted in the notification | ||
/// channel buffer. | ||
const NOTIFICATION_BUFFER_SIZE: usize = 100; | ||
|
||
// TODO(holzeis): This enum should be extended to allow for sending push notifications. | ||
pub enum Notification { | ||
Message { | ||
trader_id: PublicKey, | ||
message: Message, | ||
}, | ||
} | ||
|
||
#[derive(Clone)] | ||
pub struct NewUserMessage { | ||
pub new_user: PublicKey, | ||
pub sender: mpsc::Sender<Message>, | ||
} | ||
|
||
pub fn start( | ||
tx_user_feed: broadcast::Sender<NewUserMessage>, | ||
) -> (RemoteHandle<Result<()>>, mpsc::Sender<Notification>) { | ||
let (sender, mut receiver) = mpsc::channel::<Notification>(NOTIFICATION_BUFFER_SIZE); | ||
|
||
let authenticated_users = Arc::new(RwLock::new(HashMap::new())); | ||
|
||
tokio::task::spawn({ | ||
let traders = authenticated_users.clone(); | ||
async move { | ||
let mut user_feed = tx_user_feed.subscribe(); | ||
while let Ok(new_user_msg) = user_feed.recv().await { | ||
traders | ||
.write() | ||
.expect("RwLock to not be poisoned") | ||
.insert(new_user_msg.new_user, new_user_msg.sender); | ||
} | ||
} | ||
}); | ||
|
||
let (fut, remote_handle) = { | ||
async move { | ||
while let Some(notification) = receiver.recv().await { | ||
match notification { | ||
Notification::Message { trader_id, message } => { | ||
tracing::info!(%trader_id, "Sending message: {message:?}"); | ||
|
||
let trader = { | ||
let traders = authenticated_users | ||
.read() | ||
.expect("RwLock to not be poisoned"); | ||
traders.get(&trader_id).cloned() | ||
}; | ||
|
||
match trader { | ||
Some(sender) => { | ||
if let Err(e) = sender.send(message).await { | ||
tracing::warn!("Connection lost to trader {e:#}"); | ||
} | ||
} | ||
None => tracing::warn!(%trader_id, "Trader is not connected"), | ||
} | ||
} | ||
} | ||
} | ||
|
||
Ok(()) | ||
} | ||
.remote_handle() | ||
}; | ||
|
||
tokio::spawn(fut); | ||
|
||
(remote_handle, sender) | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Doesn't the fact that you shadow
_handle
cause the previousRemoteHandle
s to be dropped stopping the corresponding tasks?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, I think there is not much benefit to just returning the
RemoteHandle
to "ignore" it. What you usually do with aRemoteHandle
is store in some other struct so that when you drop that one the corresponding task is cancelled.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't look like it. At least it worked during testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well the reason I return it to the top level is so that the tasks live as long as the coordinator. That's why I am doing this, do you think that is not needed? Happy to remove it, but I think when I did, the compiler warned me about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed it does not drop it: https://www.reddit.com/r/rust/comments/qpekne/what_actually_happens_when_ypu_shadow_a_variable/.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess because the coordinator is the whole program it isn't actually useful. It would be handy if we could remake the coordinator in case of catastrophic failure (without restarting the binary). Then you would want to cancel all the old tasks and restart them with a fresh coordinator struct.
That's probably because you called
remote_handle
on the future and returned theRemoteHandle
which is marked asmust_use
. You can obviously just spawn the future without doing any of that, but I don't mind if you wanna keep the current version.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got ya. Since this PR is now open for quite some time. I prefer to go ahead with the current change, and adapt the
remote_handle
stuff in a follow up PR.