Repository: golemfactory/yagna (language: python)

Issue:
Creating agreement on rejected proposal
Why: Requestor's request to create an agreement on rejected proposal should fail immediately. 
What: Requestor is able to create an agreement on rejected counter proposal.
```
[2021-07-16T11:05:08.218+0200 INFO  ya_market::negotiation::common] Requestor [0x6c1a5409a97c657c4c82f8a5caa9bbe
a5c955082] rejected Proposal [R-f537d5f43b88fd753c8b1d2fa96bb0fc853ff148b26fcf3ec416c159cf11ffc1] 'Rejected'.   
[2021-07-16T11:05:13.314+0200 INFO  ya_market::negotiation::requestor] Requestor 'golem-cli' [0x6c1a5409a97c657c
4c82f8a5caa9bbea5c955082] created Agreement [R-329aed1c57279b36fdf975d2980cc1249f8cfa46dd5e1a391481a798a2940d48]
 from Proposal [R-f537d5f43b88fd753c8b1d2fa96bb0fc853ff148b26fcf3ec416c159cf11ffc1].                            
[2021-07-16T11:05:13.408+0200 INFO  ya_market::negotiation::requestor] Requestor 'golem-cli' [0x6c1a5409a97c657c
4c82f8a5caa9bbea5c955082] confirmed Agreement [R-329aed1c57279b36fdf975d2980cc1249f8cfa46dd5e1a391481a798a2940d4
8] and sent to Provider.                                                                                        
[2021-07-16T11:05:28.418+0200 ERROR ya_market::rest_api::requestor::wait_for_approval] Error occurred: Timeout w
hile waiting for Agreement [R-329aed1c57279b36fdf975d2980cc1249f8cfa46dd5e1a391481a798a2940d48] approval.
```


Discussion / hints:

Fix patch:
diff --git a/core/market/src/negotiation/error.rs b/core/market/src/negotiation/error.rs
index 38e582976d..f135e405e2 100644
--- a/core/market/src/negotiation/error.rs
+++ b/core/market/src/negotiation/error.rs
@@ -48,18 +48,20 @@ pub enum AgreementError {
     NotFound(String),
     #[error("Agreement [{0}] expired.")]
     Expired(AgreementId),
-    #[error("Can't create Agreement for Proposal {0}. Proposal {1} not found.")]
+    #[error("Can't create Agreement for Proposal [{0}]. Proposal [{1}] not found.")]
     ProposalNotFound(ProposalId, ProposalId),
-    #[error("Can't create second Agreement [{0}] for Proposal [{1}].")]
-    AlreadyExists(AgreementId, ProposalId),
-    #[error("Can't create Agreement for Proposal {0}. Failed to get Proposal {1}. Error: {2}")]
+    #[error("Can't create second Agreement for Proposal [{0}].")]
+    ProposalAlreadyAccepted(ProposalId),
+    #[error("Can't create Agreement for Proposal [{0}]. Failed to get Proposal [{1}]. Error: {2}")]
     GetProposal(ProposalId, ProposalId, String),
     #[error("Can't create Agreement for already countered Proposal [{0}].")]
     ProposalCountered(ProposalId),
-    #[error("Can't create Agreement for Proposal {0}. No negotiation with Provider took place. (You should counter Proposal at least one time)")]
+    #[error("Can't create Agreement for Proposal [{0}]. No negotiation with Provider took place. (You should counter Proposal at least one time)")]
     NoNegotiations(ProposalId),
-    #[error("Can't create Agreement for out own Proposal {0}. You can promote only provider's Proposals to Agreement.")]
+    #[error("Can't create Agreement for an own Proposal [{0}]. You can promote only provider's Proposals to Agreement.")]
     OwnProposal(ProposalId),
+    #[error("Can't create Agreement for rejected Proposal [{0}].")]
+    ProposalRejected(ProposalId),
     #[error("Failed to save Agreement for Proposal [{0}]. Error: {1}")]
     Save(ProposalId, DbError),
     #[error("Failed to get Agreement [{0}]. Error: {1}")]
diff --git a/core/market/src/negotiation/requestor.rs b/core/market/src/negotiation/requestor.rs
index 461067ce51..ee370ebed0 100644
--- a/core/market/src/negotiation/requestor.rs
+++ b/core/market/src/negotiation/requestor.rs
@@ -22,6 +22,7 @@ use crate::protocol::negotiation::{error::*, messages::*, requestor::Negotiation
 use super::{common::*, error::*, notifier::NotifierError, EventNotifier};
 use crate::config::Config;
 use crate::db::dao::AgreementEventsDao;
+use crate::db::model::ProposalState;
 use crate::utils::display::EnableDisplay;
 
 #[derive(Clone, derive_more::Display, Debug, PartialEq)]
@@ -249,6 +250,23 @@ impl RequestorBroker {
         if offer_proposal.body.issuer != Issuer::Them {
             return Err(AgreementError::OwnProposal(proposal_id.clone()));
         }
+        // We can promote only Proposals in Draft state
+        // (not Initial, nor Rejected, nor already Accepted, and of course nor already Expired)
+        match offer_proposal.body.state {
+            ProposalState::Draft => {}
+            ProposalState::Initial => {
+                return Err(AgreementError::NoNegotiations(proposal_id.clone()))
+            }
+            ProposalState::Rejected => {
+                return Err(AgreementError::ProposalRejected(proposal_id.clone()))
+            }
+            ProposalState::Accepted => {
+                return Err(AgreementError::ProposalAlreadyAccepted(proposal_id.clone()))
+            }
+            ProposalState::Expired => {
+                // TODO: expiration of proposals is never checked
+            }
+        }
 
         let demand_proposal_id = offer_proposal
             .body
@@ -276,8 +294,8 @@ impl RequestorBroker {
             .map_err(|e| match e {
                 SaveAgreementError::Internal(e) => AgreementError::Save(proposal_id.clone(), e),
                 SaveAgreementError::ProposalCountered(id) => AgreementError::ProposalCountered(id),
-                SaveAgreementError::Exists(agreement_id, proposal_id) => {
-                    AgreementError::AlreadyExists(agreement_id, proposal_id)
+                SaveAgreementError::Exists(_agreement_id, proposal_id) => {
+                    AgreementError::ProposalAlreadyAccepted(proposal_id)
                 }
             })?;
 
diff --git a/core/market/src/rest_api/error.rs b/core/market/src/rest_api/error.rs
index 41aca0ec35..a4fc9df179 100644
--- a/core/market/src/rest_api/error.rs
+++ b/core/market/src/rest_api/error.rs
@@ -194,9 +194,10 @@ impl ResponseError for AgreementError {
         match self {
             AgreementError::NotFound(_) => HttpResponse::NotFound().json(msg),
             AgreementError::Expired(_) => HttpResponse::Gone().json(msg),
-            AgreementError::AlreadyExists(_, _) => HttpResponse::Conflict().json(msg),
+            AgreementError::ProposalAlreadyAccepted(..) => HttpResponse::Conflict().json(msg),
             AgreementError::UpdateState(_, e) => e.error_response(),
             AgreementError::NoNegotiations(_)
+            | AgreementError::ProposalRejected(..)
             | AgreementError::OwnProposal(..)
             | AgreementError::ProposalNotFound(..)
             | AgreementError::ProposalCountered(..)


Test patch:
diff --git a/core/market/tests/test_agreement.rs b/core/market/tests/test_agreement.rs
index dc2cef26e6..299213cb9b 100644
--- a/core/market/tests/test_agreement.rs
+++ b/core/market/tests/test_agreement.rs
@@ -232,7 +232,7 @@ async fn second_creation_should_fail() {
     let req_id = network.get_default_id(REQ_NAME);
 
     // when: expiration time is now
-    let agreement_id = req_engine
+    let _agreement_id = req_engine
         .create_agreement(req_id.clone(), &proposal_id, Utc::now())
         .await
         .unwrap();
@@ -241,10 +241,7 @@ async fn second_creation_should_fail() {
         .create_agreement(req_id.clone(), &proposal_id, Utc::now())
         .await;
 
-    assert_err_eq!(
-        AgreementError::AlreadyExists(agreement_id, proposal_id),
-        result,
-    );
+    assert_err_eq!(AgreementError::ProposalAlreadyAccepted(proposal_id), result,);
 }
 
 #[cfg_attr(not(feature = "test-suite"), ignore)]
diff --git a/core/market/tests/test_negotiations.rs b/core/market/tests/test_negotiations.rs
index 322c39b3cf..7e2f68c610 100644
--- a/core/market/tests/test_negotiations.rs
+++ b/core/market/tests/test_negotiations.rs
@@ -8,8 +8,8 @@ use ya_market::testing::{
     mock_offer::flatten_json,
     negotiation::error::{CounterProposalError, RemoteProposalError},
     proposal_util::{exchange_draft_proposals, NegotiationHelper},
-    MarketServiceExt, MarketsNetwork, Owner, ProposalError, ProposalState, ProposalValidationError,
-    SaveProposalError,
+    AgreementError, MarketServiceExt, MarketsNetwork, Owner, ProposalError, ProposalState,
+    ProposalValidationError, SaveProposalError,
 };
 
 /// Test countering initial and draft proposals on both Provider and Requestor side.
@@ -1071,6 +1071,81 @@ async fn test_restart_negotiations() {
         .unwrap();
 }
 
+#[cfg_attr(not(feature = "test-suite"), ignore)]
+#[serial_test::serial]
+async fn test_create_agreement_on_rejected_proposal_should_fail() {
+    let network = MarketsNetwork::new(None)
+        .await
+        .add_market_instance("Requestor1")
+        .await
+        .add_market_instance("Provider1")
+        .await;
+
+    let req_market = network.get_market("Requestor1");
+    let prov_market = network.get_market("Provider1");
+    let req_id = network.get_default_id("Requestor1");
+
+    // Requestor side
+    let negotiation = exchange_draft_proposals(&network, "Requestor1", "Provider1")
+        .await
+        .unwrap();
+
+    req_market
+        .requestor_engine
+        .reject_proposal(
+            &negotiation.demand_id,
+            &negotiation.proposal_id,
+            &req_id,
+            None,
+        )
+        .await
+        .unwrap();
+
+    // Consume Rejected event. We need only 1 event in query_proposal later.
+    prov_market
+        .provider_engine
+        .query_events(&negotiation.offer_id, 3.0, Some(5))
+        .await
+        .unwrap();
+
+    // Proposal state should be `Rejected` on both sides.
+    assert_eq!(
+        prov_market
+            .get_proposal(&negotiation.proposal_id.clone().translate(Owner::Provider))
+            .await
+            .unwrap()
+            .body
+            .state,
+        ProposalState::Rejected
+    );
+    assert_eq!(
+        req_market
+            .get_proposal(&negotiation.proposal_id)
+            .await
+            .unwrap()
+            .body
+            .state,
+        ProposalState::Rejected
+    );
+
+    // Requestor do not restart negotiation by countering proposal which was rejected by him.
+    // Requestor should NOT be able to create Agreement after rejection.
+    match req_market
+        .requestor_engine
+        .create_agreement(
+            req_id.clone(),
+            &negotiation.proposal_id,
+            Utc::now() + Duration::milliseconds(300),
+        )
+        .await
+    {
+        Err(AgreementError::ProposalRejected(id)) => {
+            assert_eq!(id, negotiation.proposal_id);
+        }
+        e => panic!("Expected AgreementError::ProposalRejected, got: {:?}", e),
+    }
+}
+
 /// Agent is allowed to restart negotiations after Agreement was rejected.
 #[cfg_attr(not(feature = "test-suite"), ignore)]
 #[serial_test::serial]