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

Provider - check debit notes acceptance #897

Merged
merged 38 commits into from
Jan 26, 2021

Conversation

nieznanysprawiciel
Copy link
Contributor

@nieznanysprawiciel nieznanysprawiciel commented Dec 22, 2020

resolves: #886

  • Provider checks if Requestor accepts DebitNotes
    • Of both Provider and Requestor agree to acceptance deadline, Provider will break Agreement, if deadline was exceeded.
    • Feature is backward compatible. If Requestor doesn't set specific property, Provider won't check deadlines.
    • Deadline and Agreement expiration can is configurable through env vars.
    • Provider can specify longer Agreement expiration, if Requestor promises to accept DebitNotes.
  • Better error message: If Counter Proposal will fail on matching, error will contain not matched properties
  • Updated actix to version 0.10 because of bug, that actix didn't polled ResponseFuture.
    It would be good to update rest of actix dependencies.
  • Use AppSessionId in payments
  • Negotiators can be composed from components, each implementing only part of negotiation logic.

@nieznanysprawiciel nieznanysprawiciel force-pushed the provider/check-debit-notes-acceptance branch from 0a2757b to b698012 Compare December 23, 2020 17:24
@nieznanysprawiciel nieznanysprawiciel self-assigned this Dec 23, 2020
@tworec tworec added this to the Mkt & Prov MVP milestone Dec 28, 2020
@nieznanysprawiciel nieznanysprawiciel force-pushed the provider/check-debit-notes-acceptance branch from 2b361aa to 56b32a5 Compare January 18, 2021 14:25
@nieznanysprawiciel nieznanysprawiciel marked this pull request as ready for review January 22, 2021 12:27
Comment on lines +69 to +79
let result = self.components.negotiate_step(&proposal, offer_proposal)?;
match result {
NegotiationResult::Reject { reason } => Ok(ProposalResponse::RejectProposal { reason }),
NegotiationResult::Ready { offer } | NegotiationResult::Negotiating { offer } => {
let offer = NewOffer {
properties: serde_json::Value::Object(flatten(offer.json)),
constraints,
};
Ok(ProposalResponse::CounterProposal { offer })
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Error should result in Rejecting Proposal. (Can be done in different PR)

@tworec tworec force-pushed the provider/check-debit-notes-acceptance branch from e026270 to 8d98376 Compare January 25, 2021 17:07
},
);

self.update_deadline(ctx).unwrap();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unwrap inside

self.deadlines.insert(msg.agreement_id.to_string(), vec![]);
}

let deadlines = self.deadlines.get_mut(&msg.agreement_id).unwrap();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe entry

@tworec tworec removed request for a team and mfranciszkiewicz January 25, 2021 17:50
@tworec tworec enabled auto-merge January 26, 2021 09:37
min_agreement_expiration: std::time::Duration::from_secs(5 * 60),
max_agreement_expiration: std::time::Duration::from_secs(30 * 60),
max_agreement_expiration_without_deadline: std::time::Duration::from_secs(10 * 60),
debit_note_acceptance_deadline: std::time::Duration::from_secs(120),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
debit_note_acceptance_deadline: std::time::Duration::from_secs(120),
debit_note_acceptance_timeout: std::time::Duration::from_secs(120),

Copy link
Contributor

@tworec tworec left a comment

Choose a reason for hiding this comment

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

All of my comments are about readability only.
Still, I've tested it E2E and accept this PR in its current form.

fn debit_deadline_from(proposal: &ProposalView) -> Result<Option<Duration>> {
match proposal.pointer_typed::<i64>("/golem/com/payment/debit-notes/acceptance-timeout") {
// Requestor is able to accept DebitNotes, because he set this property.
Ok(deadline) => Ok(Some(Duration::seconds(deadline))),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Ok(deadline) => Ok(Some(Duration::seconds(deadline))),
Ok(timeout) => Ok(Some(Duration::seconds(timeout))),

Ok(Utc.timestamp_millis(timestamp))
}

fn debit_deadline_from(proposal: &ProposalView) -> Result<Option<Duration>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fn debit_deadline_from(proposal: &ProposalView) -> Result<Option<Duration>> {
fn debit_timeout_from(proposal: &ProposalView) -> Result<Option<Duration>> {

// Requestor doesn't support DebitNotes acceptance, so we should
// remove our property from Proposal to match with his.
(None, Some(_)) => {
offer.remove_property("/golem/com/payment/debit-notes/acceptance-timeout")?;
Copy link
Contributor

Choose a reason for hiding this comment

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

introduce Constant

pub payment_model: Arc<dyn PaymentModel>,
pub activities: HashMap<String, ActivityPayment>,

pub update_interval: std::time::Duration,
pub payment_deadline: Option<chrono::Duration>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pub payment_deadline: Option<chrono::Duration>,
pub payment_timeout: Option<chrono::Duration>,

@@ -95,7 +100,16 @@ impl AgreementPayment {
pub fn new(agreement: &AgreementView) -> Result<AgreementPayment> {
let payment_description = PaymentDescription::new(agreement)?;
let update_interval = payment_description.get_update_interval()?;
let payment_model = PaymentModelFactory::create(payment_description)?;
let debit_deadline = payment_description.get_debit_note_deadline()?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let debit_deadline = payment_description.get_debit_note_deadline()?;
let debit_timeout = payment_description.get_debit_note_deadline()?;

#[rtype(result = "()")]
pub struct DeadlineElapsed {
pub agreement_id: String,
pub deadline: DateTime<Utc>,
Copy link
Contributor

Choose a reason for hiding this comment

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

here the deadline is proper name, as it's describing specific point of the time

@@ -216,18 +244,38 @@ async fn send_debit_note(
&debit_note_info.activity_id
);

if let Some(deadline) = debit_note_info
.payment_deadline
.map(|deadline| Utc::now() + deadline)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.map(|deadline| Utc::now() + deadline)
.map(|timeout| Utc::now() + timeout)

@tworec tworec merged commit d7b5041 into master Jan 26, 2021
@tworec tworec deleted the provider/check-debit-notes-acceptance branch January 26, 2021 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provider: Terminate Agreement when debit notes are not received
2 participants