From 5a68e2de0b996ec2e12fea01f73d22209ff9d786 Mon Sep 17 00:00:00 2001 From: staszek-krotki <16387248+staszek-krotki@users.noreply.github.com> Date: Fri, 7 Jul 2023 11:28:18 +0200 Subject: [PATCH] handle (panic) on missing payment chosen platform (#2646) Remove rinkeby as the default payment platform --- .../provider/src/market/negotiator/builtin.rs | 1 + .../negotiator/builtin/demand_validation.rs | 139 ++++++++++++++++++ .../src/market/negotiator/composite.rs | 5 + .../provider/src/market/negotiator/factory.rs | 9 ++ core/payment/src/api/allocations.rs | 10 +- core/payment/src/dao/agreement.rs | 2 +- core/payment/src/lib.rs | 2 - core/payment/src/models/agreement.rs | 69 ++++++++- goth_tests/helpers/negotiation.py | 1 + 9 files changed, 224 insertions(+), 14 deletions(-) create mode 100644 agent/provider/src/market/negotiator/builtin/demand_validation.rs diff --git a/agent/provider/src/market/negotiator/builtin.rs b/agent/provider/src/market/negotiator/builtin.rs index 94bbc43de3..4487e15f98 100644 --- a/agent/provider/src/market/negotiator/builtin.rs +++ b/agent/provider/src/market/negotiator/builtin.rs @@ -1,3 +1,4 @@ +pub mod demand_validation; pub mod expiration; pub mod manifest; pub mod max_agreements; diff --git a/agent/provider/src/market/negotiator/builtin/demand_validation.rs b/agent/provider/src/market/negotiator/builtin/demand_validation.rs new file mode 100644 index 0000000000..6a68e0f797 --- /dev/null +++ b/agent/provider/src/market/negotiator/builtin/demand_validation.rs @@ -0,0 +1,139 @@ +use ya_agreement_utils::OfferDefinition; + +use crate::market::negotiator::factory::DemandValidationNegotiatorConfig; +use crate::market::negotiator::{ + AgreementResult, NegotiationResult, NegotiatorComponent, ProposalView, +}; + +/// Negotiator that verifies that all required fields are present in proposal. +pub struct DemandValidation { + required_fields: Vec, +} + +impl DemandValidation { + pub fn new(config: &DemandValidationNegotiatorConfig) -> DemandValidation { + let required_fields = config.required_fields.clone(); + Self { required_fields } + } +} + +impl NegotiatorComponent for DemandValidation { + fn negotiate_step( + &mut self, + demand: &ProposalView, + offer: ProposalView, + ) -> anyhow::Result { + let missing_fields = self + .required_fields + .iter() + .filter(|x| demand.pointer(x).is_none()) + .cloned() + .collect::>(); + if missing_fields.is_empty() { + Ok(NegotiationResult::Ready { offer }) + } else { + log::info!( + "'DemandValidation' negotiator: Reject proposal [{}] due to missing fields: {}", + demand.id, + missing_fields.join(",") + ); + Ok(NegotiationResult::Reject { + message: format!("Missing fields: {}", missing_fields.join(",")), + is_final: false, + }) + } + } + + fn fill_template( + &mut self, + offer_template: OfferDefinition, + ) -> anyhow::Result { + Ok(offer_template) + } + + fn on_agreement_terminated( + &mut self, + _agreement_id: &str, + _result: &AgreementResult, + ) -> anyhow::Result<()> { + Ok(()) + } + + fn on_agreement_approved(&mut self, _agreement_id: &str) -> anyhow::Result<()> { + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use chrono::Utc; + use serde_json::json; + use ya_agreement_utils::agreement::expand; + use ya_agreement_utils::OfferTemplate; + use ya_client_model::market::proposal::State; + + fn config() -> DemandValidationNegotiatorConfig { + DemandValidationNegotiatorConfig { + required_fields: vec![ + "/golem/com/freebies".to_string(), + "/golem/com/payment/address".to_string(), + ], + } + } + + fn properties_to_proposal(properties: serde_json::Value) -> ProposalView { + ProposalView { + content: OfferTemplate { + properties: expand(properties), + constraints: "()".to_string(), + }, + id: "proposalId".to_string(), + issuer: Default::default(), + state: State::Initial, + timestamp: Utc::now(), + } + } + + /// Negotiator accepts demand if all of the required fields exist + #[test] + fn test_required_fields_exist() { + let config = config(); + let mut negotiator = DemandValidation::new(&config); + + let offer = properties_to_proposal(json!({})); + let demand = properties_to_proposal(json!({ + "golem.com.freebies": "mug", + "golem.com.payment.address": "0x123", + })); + + let expected_result = NegotiationResult::Ready { + offer: offer.clone(), + }; + assert_eq!( + negotiator.negotiate_step(&demand, offer).unwrap(), + expected_result + ); + } + + /// Negotiator rejects demand if some of the required fields are missing + #[test] + fn test_required_fields_missing() { + let config = config(); + let mut negotiator = DemandValidation::new(&config); + + let offer = properties_to_proposal(json!({})); + let demand = properties_to_proposal(json!({ + "golem.com.freebies": "mug", + })); + + let expected_result = NegotiationResult::Reject { + message: "Missing fields: /golem/com/payment/address".to_string(), + is_final: false, + }; + assert_eq!( + negotiator.negotiate_step(&demand, offer).unwrap(), + expected_result + ); + } +} diff --git a/agent/provider/src/market/negotiator/composite.rs b/agent/provider/src/market/negotiator/composite.rs index abaca3df12..92a3583cca 100644 --- a/agent/provider/src/market/negotiator/composite.rs +++ b/agent/provider/src/market/negotiator/composite.rs @@ -13,6 +13,7 @@ use super::builtin::{ }; use super::common::{offer_definition_to_offer, AgreementResponse, Negotiator, ProposalResponse}; use super::{NegotiationResult, NegotiatorsPack}; +use crate::market::negotiator::builtin::demand_validation::DemandValidation; use crate::market::negotiator::common::{ reason_with_extra, AgreementFinalized, CreateOffer, ReactToAgreement, ReactToProposal, }; @@ -33,6 +34,10 @@ impl CompositeNegotiator { agent_negotiators_cfg: AgentNegotiatorsConfig, ) -> anyhow::Result { let components = NegotiatorsPack::default() + .add_component( + "Validation", + Box::new(DemandValidation::new(&config.validation_config)), + ) .add_component( "LimitAgreements", Box::new(MaxAgreements::new(&config.limit_agreements_config)), diff --git a/agent/provider/src/market/negotiator/factory.rs b/agent/provider/src/market/negotiator/factory.rs index 1ef6bca669..9a900877ed 100644 --- a/agent/provider/src/market/negotiator/factory.rs +++ b/agent/provider/src/market/negotiator/factory.rs @@ -11,6 +11,13 @@ use crate::market::negotiator::{AcceptAllNegotiator, CompositeNegotiator}; use crate::market::ProviderMarket; use crate::provider_agent::AgentNegotiatorsConfig; +/// Configuration for Demand Validation Negotiator. +#[derive(StructOpt, Clone, Debug)] +pub struct DemandValidationNegotiatorConfig { + #[structopt(long, default_value = "/golem/com/payment/chosen-platform")] + pub required_fields: Vec, +} + /// Configuration for LimitAgreements Negotiator. #[derive(StructOpt, Clone, Debug)] pub struct LimitAgreementsNegotiatorConfig { @@ -58,6 +65,8 @@ pub struct PaymentTimeoutConfig { /// Configuration for LimitAgreements Negotiator. #[derive(StructOpt, Clone, Debug)] pub struct CompositeNegotiatorConfig { + #[structopt(flatten)] + pub validation_config: DemandValidationNegotiatorConfig, #[structopt(flatten)] pub limit_agreements_config: LimitAgreementsNegotiatorConfig, #[structopt(flatten)] diff --git a/core/payment/src/api/allocations.rs b/core/payment/src/api/allocations.rs index c23064298a..876251d9dc 100644 --- a/core/payment/src/api/allocations.rs +++ b/core/payment/src/api/allocations.rs @@ -23,7 +23,6 @@ use crate::accounts::{init_account, Account}; use crate::dao::*; use crate::error::{DbError, Error}; use crate::utils::response; -use crate::DEFAULT_PAYMENT_PLATFORM; pub fn register_endpoints(scope: Scope) -> Scope { scope @@ -46,10 +45,11 @@ async fn create_allocation( // TODO: Handle deposits & timeouts let allocation = body.into_inner(); let node_id = id.identity; - let payment_platform = allocation - .payment_platform - .clone() - .unwrap_or_else(|| DEFAULT_PAYMENT_PLATFORM.to_string()); + let payment_platform = match &allocation.payment_platform { + Some(platform) => platform.clone(), + None => return response::bad_request(&"payment platform must be provided"), + }; + let address = allocation .address .clone() diff --git a/core/payment/src/dao/agreement.rs b/core/payment/src/dao/agreement.rs index 404e4aafef..bea9b1c50c 100644 --- a/core/payment/src/dao/agreement.rs +++ b/core/payment/src/dao/agreement.rs @@ -198,7 +198,7 @@ impl<'a> AgreementDao<'a> { return Ok(()); } - let agreement = WriteObj::new(agreement, role); + let agreement = WriteObj::try_new(agreement, role).map_err(DbError::Query)?; diesel::insert_into(dsl::pay_agreement) .values(agreement) .execute(conn)?; diff --git a/core/payment/src/lib.rs b/core/payment/src/lib.rs index e89262455e..1f116c209f 100644 --- a/core/payment/src/lib.rs +++ b/core/payment/src/lib.rs @@ -28,8 +28,6 @@ pub mod migrations { struct _Dummy; } -pub const DEFAULT_PAYMENT_PLATFORM: &str = "erc20-rinkeby-tglm"; - pub use ya_core_model::payment::local::DEFAULT_PAYMENT_DRIVER; lazy_static::lazy_static! { diff --git a/core/payment/src/models/agreement.rs b/core/payment/src/models/agreement.rs index ace1faa2d2..010b5beaec 100644 --- a/core/payment/src/models/agreement.rs +++ b/core/payment/src/models/agreement.rs @@ -1,5 +1,4 @@ use crate::schema::pay_agreement; -use crate::DEFAULT_PAYMENT_PLATFORM; use serde_json::Value; use ya_agreement_utils::agreement::{expand, TypedPointer}; use ya_client_model::market::Agreement; @@ -25,7 +24,7 @@ pub struct WriteObj { } impl WriteObj { - pub fn new(agreement: Agreement, role: Role) -> Self { + pub fn try_new(agreement: Agreement, role: Role) -> Result { let provider_id = *agreement.provider_id(); let requestor_id = *agreement.requestor_id(); let (owner_id, peer_id) = match &role { @@ -39,8 +38,8 @@ impl WriteObj { let payment_platform = demand_properties .pointer("/golem/com/payment/chosen-platform") .as_typed(Value::as_str) - .unwrap_or(DEFAULT_PAYMENT_PLATFORM) - .to_owned(); + .map(ToOwned::to_owned) + .map_err(|_| "Missing golem.com.payment.chosen-platform".to_string())?; let payee_addr = offer_properties .pointer(format!("/golem/com/payment/platform/{}/address", payment_platform).as_str()) .as_typed(Value::as_str) @@ -52,7 +51,7 @@ impl WriteObj { .map(ToOwned::to_owned) .unwrap_or_else(|_| requestor_id.to_string().to_lowercase()); - Self { + Ok(Self { id: agreement.agreement_id, owner_id, role, @@ -65,8 +64,66 @@ impl WriteObj { total_amount_scheduled: Default::default(), total_amount_paid: Default::default(), app_session_id: agreement.app_session_id, - } + }) } } pub type ReadObj = WriteObj; + +#[cfg(test)] +mod tests { + use crate::models::agreement::WriteObj; + use chrono::{Duration, Utc}; + use serde_json::json; + use std::ops::Add; + use ya_client_model::market::agreement::State; + use ya_client_model::market::{Agreement, Demand, Offer}; + use ya_persistence::types::Role; + + fn mock_agreement_with_demand_properties(properties: serde_json::Value) -> Agreement { + let demand = Demand::new( + properties, + "()".to_string(), + "demand_id".to_string(), + Default::default(), + Default::default(), + ); + + let offer = Offer::new( + json!({}), + "()".to_string(), + "offer_id".to_string(), + Default::default(), + Default::default(), + ); + + Agreement::new( + "agreement_id".to_string(), + demand, + offer, + Utc::now().add(Duration::days(1)), + State::Proposal, + Utc::now(), + ) + } + + #[test] + fn cannot_create_agreement_without_chosen_platform() { + let agreement = mock_agreement_with_demand_properties(json!({})); + let role: Role = Role::Provider; + + let result = WriteObj::try_new(agreement, role); + assert!(result.is_err()); + } + + #[test] + fn create_agreement_with_valid_chosen_platform() { + let agreement = mock_agreement_with_demand_properties(json!({ + "golem.com.payment.chosen-platform": "test-network" + })); + let role: Role = Role::Provider; + + let result = WriteObj::try_new(agreement, role); + assert_eq!(result.unwrap().payment_platform, "test-network"); + } +} diff --git a/goth_tests/helpers/negotiation.py b/goth_tests/helpers/negotiation.py index dc464a7c39..6730c9a35c 100644 --- a/goth_tests/helpers/negotiation.py +++ b/goth_tests/helpers/negotiation.py @@ -36,6 +36,7 @@ def props_from_template(self, task_package: Optional[str]) -> "DemandBuilder": "golem.srv.comp.expiration": int( (datetime.now() + timedelta(minutes=10)).timestamp() * 1000 ), + "golem.com.payment.chosen-platform": self._requestor.payment_config.platform_string, } if task_package is not None: