-
Notifications
You must be signed in to change notification settings - Fork 36
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
wip(feat): reissue ecash notes from OOBNotes
#1037
base: master
Are you sure you want to change the base?
wip(feat): reissue ecash notes from OOBNotes
#1037
Conversation
5866f3c
to
72fda2a
Compare
ed128ab
to
2ac7972
Compare
mutiny-core/src/federation.rs
Outdated
|
||
// TODO: (@leonardo) re-think about the results and errors that we need/want | ||
match process_reissue_outcome(&mint_module, operation_id, logger.clone()).await? { | ||
ReissueExternalNotesState::Created | ReissueExternalNotesState::Failed(_) => { |
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.
Why return error on created
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'm not sure how to proceed with this, but if the stream is already finished and the final result is still Created, it didn't get processed by the federation without a real guaranteeing it will.
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'll need to check in the discord about this.
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.
AFAIK there is no known error it would still be stuck at created even after all the stream is consumed, but still needs to handle the variant 🤔
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.
@benthecarman What do you think we should do here with Created variant?
It's not supposed to have that after the whole stream is consumed, so still handle as failure/error ? 🤔
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.
in process_reissue_outcome
you only return the outcome if it is Failed
or Done
so you should just handle those two here, and leave the rest in the _ =>
case where you can throw an error.
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.
wdyt about eb59a8b
2f8abbb
to
03d499d
Compare
OOBNotes
`OOBNotes
03497a7
to
ef9545d
Compare
ef9545d
to
834b6c8
Compare
wip(feat): reissue ecash notes from `OOBNotes`` wip: working version 🚀 fix: clippy
834b6c8
to
41760d9
Compare
mutiny-core/src/lib.rs
Outdated
@@ -119,6 +119,7 @@ use crate::utils::parse_profile_metadata; | |||
use mockall::{automock, predicate::*}; | |||
|
|||
const DEFAULT_PAYMENT_TIMEOUT: u64 = 30; | |||
const DEFAULT_REISSUE_TIMEOUT: u64 = 50; |
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.
this is only used in federation.rs, would be better to define in there. This should probably go back to 30 seconds too. you're also always casting to i32
, can just define it as such
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.
nice, done!
but I kept it as u64 to follow the standard, or should I also change the other ones? 🤔
mutiny-core/src/federation.rs
Outdated
); | ||
return Ok(outcome); | ||
} | ||
_ => { /* ignore and continue */ } |
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.
would be helpful to log each state
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.
nice, I'll add it :)
eb59a8b
to
24d7611
Compare
No description provided.