From 10deba9789a5bc324ca30222c490b0233e4850ea Mon Sep 17 00:00:00 2001 From: Lachezar Lechev Date: Wed, 4 Aug 2021 17:35:09 +0300 Subject: [PATCH 01/10] sentry - mod test_util - setup_dummy_app helper fn --- sentry/src/lib.rs | 47 +++++++++++++++++++++++++++++++ sentry/src/middleware/campaign.rs | 45 ++--------------------------- 2 files changed, 50 insertions(+), 42 deletions(-) diff --git a/sentry/src/lib.rs b/sentry/src/lib.rs index c5ff2ef83..ca5bde526 100644 --- a/sentry/src/lib.rs +++ b/sentry/src/lib.rs @@ -500,3 +500,50 @@ pub struct Auth { pub era: i64, pub uid: ValidatorId, } + +#[cfg(test)] +pub mod test_util { + use adapter::DummyAdapter; + use primitives::{ + adapter::DummyAdapterOptions, + config::configuration, + util::tests::{ + discard_logger, + prep_db::{IDS}, + }, + }; + + use crate::{Application, db::{ + redis_pool::TESTS_POOL, + tests_postgres::{setup_test_migrations, DATABASE_POOL}, + }}; + + pub async fn setup_dummy_app() -> Application { + let config = configuration("development", None).expect("Should get Config"); + let adapter = DummyAdapter::init( + DummyAdapterOptions { + dummy_identity: IDS["leader"], + dummy_auth: Default::default(), + dummy_auth_tokens: Default::default(), + }, + &config, + ); + + let redis = TESTS_POOL.get().await.expect("Should return Object"); + let database = DATABASE_POOL.get().await.expect("Should get a DB pool"); + + setup_test_migrations(database.pool.clone()) + .await + .expect("Migrations should succeed"); + + let app = Application::new( + adapter, + config, + discard_logger(), + redis.connection.clone(), + database.pool.clone(), + ); + + app + } +} diff --git a/sentry/src/middleware/campaign.rs b/sentry/src/middleware/campaign.rs index 42d16b281..024dc1400 100644 --- a/sentry/src/middleware/campaign.rs +++ b/sentry/src/middleware/campaign.rs @@ -37,57 +37,18 @@ impl Middleware for CampaignLoad { #[cfg(test)] mod test { - use adapter::DummyAdapter; use primitives::{ - adapter::DummyAdapterOptions, - config::configuration, - util::tests::{ - discard_logger, - prep_db::{DUMMY_CAMPAIGN, IDS}, - }, + util::tests::prep_db::{DUMMY_CAMPAIGN, IDS}, Campaign, }; - use crate::db::{ - insert_campaign, - redis_pool::TESTS_POOL, - tests_postgres::{setup_test_migrations, DATABASE_POOL}, - }; + use crate::{db::insert_campaign, test_util::setup_dummy_app}; use super::*; - async fn setup_app() -> Application { - let config = configuration("development", None).expect("Should get Config"); - let adapter = DummyAdapter::init( - DummyAdapterOptions { - dummy_identity: IDS["leader"], - dummy_auth: Default::default(), - dummy_auth_tokens: Default::default(), - }, - &config, - ); - - let redis = TESTS_POOL.get().await.expect("Should return Object"); - let database = DATABASE_POOL.get().await.expect("Should get a DB pool"); - - setup_test_migrations(database.pool.clone()) - .await - .expect("Migrations should succeed"); - - let app = Application::new( - adapter, - config, - discard_logger(), - redis.connection.clone(), - database.pool.clone(), - ); - - app - } - #[tokio::test] async fn campaign_loading() { - let app = setup_app().await; + let app = setup_dummy_app().await; let build_request = |params: RouteParams| { Request::builder() From 88c324331be45e4ee463487f3d8e036f7882a3c6 Mon Sep 17 00:00:00 2001 From: Lachezar Lechev Date: Wed, 4 Aug 2021 17:36:46 +0300 Subject: [PATCH 02/10] sentry - db - campaign - CampaignRemaining redis struct --- sentry/src/db/campaign.rs | 251 +++++++++++++++++++++++++++++++++- sentry/src/routes/campaign.rs | 233 ++++++++----------------------- 2 files changed, 307 insertions(+), 177 deletions(-) diff --git a/sentry/src/db/campaign.rs b/sentry/src/db/campaign.rs index fed600754..9c5287a20 100644 --- a/sentry/src/db/campaign.rs +++ b/sentry/src/db/campaign.rs @@ -2,10 +2,16 @@ use crate::db::{DbPool, PoolError}; use primitives::{Campaign, CampaignId, ChannelId}; use tokio_postgres::types::Json; +pub use campaign_remaining::CampaignRemaining; + +/// ```text +/// INSERT INTO campaigns (id, channel_id, channel, creator, budget, validators, title, pricing_bounds, event_submission, ad_units, targeting_rules, created, active_from, active_to) +/// VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14) +/// ``` pub async fn insert_campaign(pool: &DbPool, campaign: &Campaign) -> Result { let client = pool.get().await?; let ad_units = Json(campaign.ad_units.clone()); - let stmt = client.prepare("INSERT INTO campaigns (id, channel_id, channel, creator, budget, validators, title, pricing_bounds, event_submission, ad_units, targeting_rules, created, active_from, active_to) values ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14)").await?; + let stmt = client.prepare("INSERT INTO campaigns (id, channel_id, channel, creator, budget, validators, title, pricing_bounds, event_submission, ad_units, targeting_rules, created, active_from, active_to) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14)").await?; let inserted = client .execute( &stmt, @@ -49,6 +55,10 @@ pub async fn fetch_campaign( } // TODO: We might need to use LIMIT to implement pagination +/// ```text +/// SELECT id, channel, creator, budget, validators, title, pricing_bounds, event_submission, ad_units, targeting_rules, created, active_from, active_to +/// FROM campaigns WHERE channel_id = $1 +/// ``` pub async fn get_campaigns_by_channel( pool: &DbPool, channel_id: &ChannelId, @@ -95,6 +105,245 @@ pub async fn update_campaign(pool: &DbPool, campaign: &Campaign) -> Result String { + format!("{}:{}", Self::CAMPAIGN_REMAINING_KEY, campaign) + } + + pub fn new(redis: MultiplexedConnection) -> Self { + Self { redis } + } + + pub async fn set_initial( + &self, + campaign: CampaignId, + amount: UnifiedNum, + ) -> Result { + redis::cmd("SETNX") + .arg(&Self::get_key(campaign)) + .arg(amount.to_u64()) + .query_async(&mut self.redis.clone()) + .await + } + + pub async fn get_remaining_opt( + &self, + campaign: CampaignId, + ) -> Result, RedisError> { + redis::cmd("GET") + .arg(&Self::get_key(campaign)) + .query_async::<_, Option>(&mut self.redis.clone()) + .await + } + + /// This method uses `max(0, value)` to clamp the value of a campaign, which can be negative and uses `i64`. + /// In addition, it defaults the campaign keys that were not found to `0`. + pub async fn get_multiple( + &self, + campaigns: &[CampaignId], + ) -> Result, RedisError> { + let keys: Vec = campaigns + .iter() + .map(|campaign| Self::get_key(*campaign)) + .collect(); + + let campaigns_remaining = redis::cmd("MGET") + .arg(keys) + .query_async::<_, Vec>>(&mut self.redis.clone()) + .await? + .into_iter() + .map(|remaining| match remaining { + Some(remaining) => UnifiedNum::from_u64(remaining.max(0).unsigned_abs()), + None => UnifiedNum::from_u64(0), + }) + .collect(); + + Ok(campaigns_remaining) + } + + pub async fn increase_by( + &self, + campaign: CampaignId, + amount: UnifiedNum, + ) -> Result { + let key = Self::get_key(campaign); + redis::cmd("INCRBY") + .arg(&key) + .arg(amount.to_u64()) + .query_async(&mut self.redis.clone()) + .await + } + + pub async fn decrease_by( + &self, + campaign: CampaignId, + amount: UnifiedNum, + ) -> Result { + let key = Self::get_key(campaign); + redis::cmd("DECRBY") + .arg(&key) + .arg(amount.to_u64()) + .query_async(&mut self.redis.clone()) + .await + } + } + + #[cfg(test)] + mod test { + use primitives::util::tests::prep_db::DUMMY_CAMPAIGN; + + use crate::db::redis_pool::TESTS_POOL; + + use super::*; + + #[tokio::test] + async fn it_sets_initial_increases_and_decreases_remaining_for_campaign() { + let redis = TESTS_POOL.get().await.expect("Should return Object"); + + let campaign = DUMMY_CAMPAIGN.id; + let campaign_remaining = CampaignRemaining::new(redis.connection.clone()); + + // Get remaining on a key which was not set + { + let get_remaining = campaign_remaining + .get_remaining_opt(campaign) + .await + .expect("Should get None"); + + assert_eq!(None, get_remaining); + } + + // Set Initial amount on that key + { + let initial_amount = UnifiedNum::from(1_000_u64); + let set_initial = campaign_remaining + .set_initial(campaign, initial_amount) + .await + .expect("Should set value in redis"); + assert!(set_initial); + + // get the remaining of that key, should be the initial value + let get_remaining = campaign_remaining + .get_remaining_opt(campaign) + .await + .expect("Should get None"); + + assert_eq!( + Some(1_000_i64), + get_remaining, + "should return the initial value that was set" + ); + } + + // Set initial on already existing key, should return `false` + { + let set_failing_initial = campaign_remaining + .set_initial(campaign, UnifiedNum::from(999_u64)) + .await + .expect("Should set value in redis"); + assert!(!set_failing_initial); + } + + // Decrease by amount + { + let decrease_amount = UnifiedNum::from(64); + let decrease_by = campaign_remaining + .decrease_by(campaign, decrease_amount) + .await + .expect("Should decrease remaining amount"); + + assert_eq!(936_i64, decrease_by); + } + + // Increase by amount + { + let increase_amount = UnifiedNum::from(1064); + let increase_by = campaign_remaining + .increase_by(campaign, increase_amount) + .await + .expect("Should increase remaining amount"); + + assert_eq!(2_000_i64, increase_by); + } + + let get_remaining = campaign_remaining + .get_remaining_opt(campaign) + .await + .expect("Should get remaining"); + + assert_eq!(Some(2_000_i64), get_remaining); + + // Decrease by amount > than currently set + { + let decrease_amount = UnifiedNum::from(5_000); + let decrease_by = campaign_remaining + .decrease_by(campaign, decrease_amount) + .await + .expect("Should decrease remaining amount"); + + assert_eq!(-3_000_i64, decrease_by); + } + + // Increase the negative value without going > 0 + { + let increase_amount = UnifiedNum::from(1000); + let increase_by = campaign_remaining + .increase_by(campaign, increase_amount) + .await + .expect("Should increase remaining amount"); + + assert_eq!(-2_000_i64, increase_by); + } + } + + #[tokio::test] + async fn it_gets_multiple_campaigns_remaining() { + let redis = TESTS_POOL.get().await.expect("Should return Object"); + let campaign_remaining = CampaignRemaining::new(redis.connection.clone()); + + let campaigns = (CampaignId::new(), CampaignId::new(), CampaignId::new()); + + // set initial amounts + { + assert!(campaign_remaining + .set_initial(campaigns.0, UnifiedNum::from(100)) + .await + .expect("Should set value in redis")); + + assert!(campaign_remaining + .set_initial(campaigns.1, UnifiedNum::from(200)) + .await + .expect("Should set value in redis")); + + assert!(campaign_remaining + .set_initial(campaigns.2, UnifiedNum::from(300)) + .await + .expect("Should set value in redis")); + } + + // set campaigns.1 to negative value, should return `0` because of `max(value, 0)` + assert_eq!(-300_i64, campaign_remaining.decrease_by(campaigns.1, UnifiedNum::from(500)).await.expect("Should decrease remaining")); + + let multiple = campaign_remaining.get_multiple(&[campaigns.0, campaigns.1, campaigns.2]).await.expect("Should get multiple"); + + assert_eq!(vec![UnifiedNum::from(100), UnifiedNum::from(0), UnifiedNum::from(300)], multiple); + } + } +} + #[cfg(test)] mod test { use primitives::{ diff --git a/sentry/src/routes/campaign.rs b/sentry/src/routes/campaign.rs index f7612dd04..6d57607b0 100644 --- a/sentry/src/routes/campaign.rs +++ b/sentry/src/routes/campaign.rs @@ -1,14 +1,4 @@ -use crate::{ - access::{self, check_access}, - db::{ - accounting::get_accounting_spent, - campaign::{get_campaigns_by_channel, insert_campaign, update_campaign}, - spendable::fetch_spendable, - DbPool, - }, - routes::campaign::update_campaign::set_initial_remaining_for_campaign, - success_response, Application, Auth, ResponseError, Session, -}; +use crate::{Application, Auth, ResponseError, Session, access::{self, check_access}, db::{CampaignRemaining, DbPool, RedisError, accounting::get_accounting_spent, campaign::{get_campaigns_by_channel, insert_campaign, update_campaign}, spendable::fetch_spendable}, success_response}; use chrono::Utc; use deadpool_postgres::PoolError; use hyper::{Body, Request, Response}; @@ -19,9 +9,8 @@ use primitives::{ campaign_create::{CreateCampaign, ModifyCampaign}, Event, SuccessResponse, }, - Address, Campaign, CampaignId, UnifiedNum, + Address, Campaign, UnifiedNum, }; -use redis::{aio::MultiplexedConnection, RedisError}; use slog::error; use std::{ cmp::{max, Ordering}, @@ -69,7 +58,7 @@ pub async fn create_campaign( campaign .validate(&app.config, &app.adapter.whoami()) - .map_err(|_| ResponseError::FailedValidation("couldn't valdiate campaign".to_string()))?; + .map_err(|err| ResponseError::FailedValidation(err.to_string()))?; if auth.uid.to_address() != campaign.creator { return Err(ResponseError::Forbidden( @@ -105,14 +94,22 @@ pub async fn create_campaign( } // If the campaign is being created, the amount spent is 0, therefore remaining = budget - set_initial_remaining_for_campaign(&app.redis, campaign.id, campaign.budget) + let remaining_set = CampaignRemaining::new(app.redis.clone()).set_initial(campaign.id, campaign.budget) .await .map_err(|_| { ResponseError::BadRequest( - "Couldn't update remaining while creating campaign".to_string(), + "Couldn't set remaining while creating campaign".to_string(), ) })?; + // If for some reason the randomly generated `CampaignId` exists in Redis + // This should **NOT** happen! + if !remaining_set { + return Err(ResponseError::Conflict( + "The generated CampaignId already exists, please repeat the request".to_string(), + )) + } + // insert Campaign match insert_campaign(&app.pool, &campaign).await { Err(error) => { @@ -136,50 +133,9 @@ pub async fn create_campaign( } pub mod update_campaign { - use super::*; - - pub const CAMPAIGN_REMAINING_KEY: &'static str = "campaignRemaining"; - - pub async fn set_initial_remaining_for_campaign( - redis: &MultiplexedConnection, - id: CampaignId, - amount: UnifiedNum, - ) -> Result { - let key = format!("{}:{}", CAMPAIGN_REMAINING_KEY, id); - redis::cmd("SETNX") - .arg(&key) - .arg(amount.to_u64()) - .query_async(&mut redis.clone()) - .await?; - Ok(true) - } - - pub async fn increase_remaining_for_campaign( - redis: &MultiplexedConnection, - id: CampaignId, - amount: UnifiedNum, - ) -> Result { - let key = format!("{}:{}", CAMPAIGN_REMAINING_KEY, id); - redis::cmd("INCRBY") - .arg(&key) - .arg(amount.to_u64()) - .query_async::<_, u64>(&mut redis.clone()) - .await - .map(UnifiedNum::from) - } + use crate::db::CampaignRemaining; - pub async fn decrease_remaining_for_campaign( - redis: &MultiplexedConnection, - id: CampaignId, - amount: UnifiedNum, - ) -> Result { - let key = format!("{}:{}", CAMPAIGN_REMAINING_KEY, id); - redis::cmd("DECRBY") - .arg(&key) - .arg(amount.to_u64()) - .query_async::<_, i64>(&mut redis.clone()) - .await - } + use super::*; pub async fn handle_route( req: Request, @@ -196,10 +152,12 @@ pub mod update_campaign { let modify_campaign_fields = serde_json::from_slice::(&body) .map_err(|e| ResponseError::FailedValidation(e.to_string()))?; + let campaign_remaining = CampaignRemaining::new(app.redis.clone()); + // modify Campaign let modified_campaign = modify_campaign( &app.pool, - &mut app.redis.clone(), + campaign_remaining, campaign_being_mutated, modify_campaign_fields, ) @@ -211,7 +169,7 @@ pub mod update_campaign { pub async fn modify_campaign( pool: &DbPool, - redis: &MultiplexedConnection, + campaign_remaining: CampaignRemaining, campaign: Campaign, modify_campaign: ModifyCampaign, ) -> Result { @@ -220,7 +178,7 @@ pub mod update_campaign { // *NOTE*: To close a campaign set campaignBudget to campaignSpent so that spendable == 0 let delta_budget = if let Some(new_budget) = modify_campaign.budget { - get_delta_budget(redis, &campaign, new_budget).await? + get_delta_budget(&campaign_remaining, &campaign, new_budget).await? } else { None }; @@ -245,15 +203,19 @@ pub mod update_campaign { let total_remaining = total_deposited .checked_sub(&accounting_spent) .ok_or(Error::Calculation)?; - let channel_campaigns = get_campaigns_by_channel(&pool, &campaign.channel.id()).await?; + let channel_campaigns = get_campaigns_by_channel(&pool, &campaign.channel.id()) + .await? + .iter() + .map(|c| c.id) + .collect::>(); // this will include the Campaign we are currently modifying - let campaigns_current_remaining_sum = - get_remaining_for_multiple_campaigns(&redis, &channel_campaigns) - .await? - .iter() - .sum::>() - .ok_or(Error::Calculation)?; + let campaigns_current_remaining_sum = campaign_remaining + .get_multiple(&channel_campaigns) + .await? + .iter() + .sum::>() + .ok_or(Error::Calculation)?; // apply the delta_budget to the sum let new_campaigns_remaining = match delta_budget { @@ -270,19 +232,18 @@ pub mod update_campaign { return Err(Error::CampaignNotModified); } - // if the value is not positive it will return an error because of UnifiedNum + // there is a chance that the new remaining will be negative even when increasing the budget + // We don't currently use this value but can be used to perform additional checks or return messages accordingly let _campaign_remaining = match delta_budget { - // should always be positive DeltaBudget::Increase(increase_by) => { - increase_remaining_for_campaign(redis, campaign.id, increase_by).await? + campaign_remaining + .increase_by(campaign.id, increase_by) + .await? } - // there is a chance that an even lowered the remaining and it's no longer positive - // check if positive and create an UnifiedNum, or return an error DeltaBudget::Decrease(decrease_by) => { - match decrease_remaining_for_campaign(redis, campaign.id, decrease_by).await? { - remaining if remaining >= 0 => UnifiedNum::from(remaining.unsigned_abs()), - _ => UnifiedNum::from(0), - } + campaign_remaining + .decrease_by(campaign.id, decrease_by) + .await? } }; } @@ -303,7 +264,7 @@ pub mod update_campaign { } async fn get_delta_budget( - redis: &MultiplexedConnection, + campaign_remaining: &CampaignRemaining, campaign: &Campaign, new_budget: UnifiedNum, ) -> Result>, Error> { @@ -316,8 +277,10 @@ pub mod update_campaign { Ordering::Less => DeltaBudget::Decrease(()), }; - let old_remaining = get_remaining_for_campaign(redis, campaign.id) + let old_remaining = campaign_remaining + .get_remaining_opt(campaign.id) .await? + .map(|remaining| UnifiedNum::from(max(0, remaining).unsigned_abs())) .ok_or(Error::FailedUpdate( "No remaining entry for campaign".to_string(), ))?; @@ -362,44 +325,6 @@ pub mod update_campaign { Ok(Some(budget)) } - - pub async fn get_remaining_for_campaign( - redis: &MultiplexedConnection, - id: CampaignId, - ) -> Result, RedisError> { - let key = format!("{}:{}", CAMPAIGN_REMAINING_KEY, id); - - let remaining = redis::cmd("GET") - .arg(&key) - .query_async::<_, Option>(&mut redis.clone()) - .await? - .map(|remaining| UnifiedNum::from(max(0, remaining).unsigned_abs())); - - Ok(remaining) - } - - async fn get_remaining_for_multiple_campaigns( - redis: &MultiplexedConnection, - campaigns: &[Campaign], - ) -> Result, Error> { - let keys: Vec = campaigns - .iter() - .map(|c| format!("{}:{}", CAMPAIGN_REMAINING_KEY, c.id)) - .collect(); - - let remainings = redis::cmd("MGET") - .arg(keys) - .query_async::<_, Vec>>(&mut redis.clone()) - .await? - .into_iter() - .map(|remaining| match remaining { - Some(remaining) => UnifiedNum::from_u64(max(0, remaining).unsigned_abs()), - None => UnifiedNum::from_u64(0), - }) - .collect(); - - Ok(remainings) - } } pub async fn insert_events( @@ -479,67 +404,23 @@ async fn process_events( mod test { use super::*; use crate::{ - campaign::update_campaign::{increase_remaining_for_campaign, CAMPAIGN_REMAINING_KEY}, - db::redis_pool::TESTS_POOL, + db::{redis_pool::TESTS_POOL, tests_postgres::DATABASE_POOL}, }; + use adapter::DummyAdapter; use primitives::util::tests::prep_db::DUMMY_CAMPAIGN; #[tokio::test] - async fn does_it_increase_remaining() { - let mut redis = TESTS_POOL.get().await.expect("Should return Object"); - let campaign = DUMMY_CAMPAIGN.clone(); - let key = format!("{}:{}", CAMPAIGN_REMAINING_KEY, campaign.id); - - // Setting the redis base variable - redis::cmd("SET") - .arg(&key) - .arg(100_u64) - .query_async::<_, ()>(&mut redis.connection) - .await - .expect("should set"); - - // 2 async calls at once, should be 500 after them - futures::future::try_join_all([ - increase_remaining_for_campaign(&redis, campaign.id, UnifiedNum::from_u64(200)), - increase_remaining_for_campaign(&redis, campaign.id, UnifiedNum::from_u64(200)), - ]) - .await - .expect("Should increase remaining twice"); - - let remaining = redis::cmd("GET") - .arg(&key) - .query_async::<_, Option>(&mut redis.connection) - .await - .expect("should get remaining"); - assert_eq!( - remaining.map(UnifiedNum::from_u64), - Some(UnifiedNum::from_u64(500)) - ); - - increase_remaining_for_campaign(&redis, campaign.id, campaign.budget) - .await - .expect("should increase"); - - let remaining = redis::cmd("GET") - .arg(&key) - // Directly parsing to u64 as we know it will be >0 - .query_async::<_, Option>(&mut redis.connection) - .await - .expect("should get remaining"); - - let should_be_remaining = UnifiedNum::from_u64(500) + campaign.budget; - assert_eq!(remaining.map(UnifiedNum::from), Some(should_be_remaining)); - - increase_remaining_for_campaign(&redis, campaign.id, UnifiedNum::from_u64(0)) - .await - .expect("should increase remaining"); - - let remaining = redis::cmd("GET") - .arg(&key) - .query_async::<_, Option>(&mut redis.connection) - .await - .expect("should get remaining"); - - assert_eq!(remaining.map(UnifiedNum::from), Some(should_be_remaining)); + /// Test single campaign creation and modification + // & + /// Test with multiple campaigns (because of Budget) a modification of campaign + async fn create_and_modify_with_multiple_campaigns() { + todo!() + // let redis = TESTS_POOL.get().await.expect("Should get redis"); + // let postgres = DATABASE_POOL.get().await.expect("Should get postgres"); + // let campaign = DUMMY_CAMPAIGN.clone(); + + // let app = Application::new() + + } } From 3ca6ab56264fb5ca6c23b149706ebe6adc652ed6 Mon Sep 17 00:00:00 2001 From: Lachezar Lechev Date: Wed, 4 Aug 2021 18:41:40 +0300 Subject: [PATCH 03/10] primitives - sentry - impl From for CreateCampaign --- primitives/src/sentry.rs | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/primitives/src/sentry.rs b/primitives/src/sentry.rs index 6018dc235..ba07b4035 100644 --- a/primitives/src/sentry.rs +++ b/primitives/src/sentry.rs @@ -361,6 +361,26 @@ pub mod campaign_create { } } + /// This implementation helps with test setup + /// **NOTE:** It erases the CampaignId, since the creation of the campaign gives it's CampaignId + impl From for CreateCampaign { + fn from(campaign: Campaign) -> Self { + Self { + channel: campaign.channel, + creator: campaign.creator, + budget: campaign.budget, + validators: campaign.validators, + title: campaign.title, + pricing_bounds: campaign.pricing_bounds, + event_submission: campaign.event_submission, + ad_units: campaign.ad_units, + targeting_rules: campaign.targeting_rules, + created: campaign.created, + active: campaign.active, + } + } + } + // All editable fields stored in one place, used for checking when a budget is changed #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] pub struct ModifyCampaign { @@ -403,7 +423,7 @@ pub mod campaign_create { if let Some(new_pricing_bounds) = self.pricing_bounds { campaign.pricing_bounds = Some(new_pricing_bounds); } - + if let Some(new_event_submission) = self.event_submission { campaign.event_submission = Some(new_event_submission); } From 11dc71564d960796801c967aa92014d7ba0cbff2 Mon Sep 17 00:00:00 2001 From: Lachezar Lechev Date: Wed, 4 Aug 2021 18:42:05 +0300 Subject: [PATCH 04/10] primitives - validator - From
for ValidatorID --- primitives/src/validator.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/primitives/src/validator.rs b/primitives/src/validator.rs index 81452b5c9..72b89f7a1 100644 --- a/primitives/src/validator.rs +++ b/primitives/src/validator.rs @@ -39,6 +39,12 @@ impl From<&Address> for ValidatorId { } } +impl From
for ValidatorId { + fn from(address: Address) -> Self { + Self(address) + } +} + impl From<&[u8; 20]> for ValidatorId { fn from(bytes: &[u8; 20]) -> Self { Self(Address::from(bytes)) From 07d9c74c969b243060800d0f883800e3799bb46d Mon Sep 17 00:00:00 2001 From: Lachezar Lechev Date: Mon, 9 Aug 2021 11:12:25 +0300 Subject: [PATCH 05/10] primitives - sentry - accounting - Balances - add spender/earner --- primitives/src/sentry/accounting.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/primitives/src/sentry/accounting.rs b/primitives/src/sentry/accounting.rs index 4b9256cec..e04167eb9 100644 --- a/primitives/src/sentry/accounting.rs +++ b/primitives/src/sentry/accounting.rs @@ -68,6 +68,16 @@ impl Balances { Ok(()) } + + /// Adds the spender to the Balances with `UnifiedNum::from(0)` if he does not exist + pub fn add_spender(&mut self, spender: Address) { + self.spenders.entry(spender).or_insert(UnifiedNum::from(0)); + } + + /// Adds the earner to the Balances with `UnifiedNum::from(0)` if he does not exist + pub fn add_earner(&mut self, earner: Address) { + self.earners.entry(earner).or_insert(UnifiedNum::from(0)); + } } #[derive(Debug)] From d80e109c61fbefe392f091f32cd883b5bca81ea6 Mon Sep 17 00:00:00 2001 From: Lachezar Lechev Date: Mon, 9 Aug 2021 11:13:31 +0300 Subject: [PATCH 06/10] sentry - Applicaiton - add CampaignRemaining to app --- sentry/src/lib.rs | 17 +++++++++++------ sentry/src/main.rs | 7 ++++--- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/sentry/src/lib.rs b/sentry/src/lib.rs index ca5bde526..0d2126a76 100644 --- a/sentry/src/lib.rs +++ b/sentry/src/lib.rs @@ -7,6 +7,7 @@ use crate::routes::channel::channel_status; use crate::routes::event_aggregate::list_channel_event_aggregates; use crate::routes::validator_message::{extract_params, list_validator_messages}; use chrono::Utc; +use db::CampaignRemaining; use hyper::{Body, Method, Request, Response, StatusCode}; use lazy_static::lazy_static; use middleware::{ @@ -90,10 +91,11 @@ impl RouteParams { #[derive(Clone)] pub struct Application { pub adapter: A, + pub config: Config, pub logger: Logger, pub redis: MultiplexedConnection, pub pool: DbPool, - pub config: Config, + pub campaign_remaining: CampaignRemaining, } impl Application { @@ -103,6 +105,7 @@ impl Application { logger: Logger, redis: MultiplexedConnection, pool: DbPool, + campaign_remaining: CampaignRemaining ) -> Self { Self { adapter, @@ -110,6 +113,7 @@ impl Application { logger, redis, pool, + campaign_remaining, } } @@ -513,13 +517,11 @@ pub mod test_util { }, }; - use crate::{Application, db::{ - redis_pool::TESTS_POOL, - tests_postgres::{setup_test_migrations, DATABASE_POOL}, - }}; + use crate::{Application, db::{CampaignRemaining, redis_pool::TESTS_POOL, tests_postgres::{setup_test_migrations, DATABASE_POOL}}}; + /// Uses production configuration to setup the correct Contract addresses for tokens. pub async fn setup_dummy_app() -> Application { - let config = configuration("development", None).expect("Should get Config"); + let config = configuration("production", None).expect("Should get Config"); let adapter = DummyAdapter::init( DummyAdapterOptions { dummy_identity: IDS["leader"], @@ -536,12 +538,15 @@ pub mod test_util { .await .expect("Migrations should succeed"); + let campaign_remaining = CampaignRemaining::new(redis.connection.clone()); + let app = Application::new( adapter, config, discard_logger(), redis.connection.clone(), database.pool.clone(), + campaign_remaining ); app diff --git a/sentry/src/main.rs b/sentry/src/main.rs index f8963f562..634831e5d 100644 --- a/sentry/src/main.rs +++ b/sentry/src/main.rs @@ -10,7 +10,7 @@ use primitives::adapter::{Adapter, DummyAdapterOptions, KeystoreOptions}; use primitives::config::configuration; use primitives::util::tests::prep_db::{AUTH, IDS}; use primitives::ValidatorId; -use sentry::db::{postgres_connection, redis_connection, setup_migrations}; +use sentry::db::{CampaignRemaining, postgres_connection, redis_connection, setup_migrations}; use sentry::Application; use slog::{error, info, Logger}; use std::{ @@ -115,18 +115,19 @@ async fn main() -> Result<(), Box> { // Check connection and setup migrations before setting up Postgres setup_migrations(&environment).await; let postgres = postgres_connection(42).await; + let campaign_remaining = CampaignRemaining::new(redis.clone()); match adapter { AdapterTypes::EthereumAdapter(adapter) => { run( - Application::new(*adapter, config, logger, redis, postgres), + Application::new(*adapter, config, logger, redis, postgres, campaign_remaining), socket_addr, ) .await } AdapterTypes::DummyAdapter(adapter) => { run( - Application::new(*adapter, config, logger, redis, postgres), + Application::new(*adapter, config, logger, redis, postgres, campaign_remaining), socket_addr, ) .await From e1a245ed20d7008f471a4cdca7462587b001ad39 Mon Sep 17 00:00:00 2001 From: Lachezar Lechev Date: Mon, 9 Aug 2021 11:14:41 +0300 Subject: [PATCH 07/10] sentry - db - CampaignRemaining - MGET guard against empty vec of campaigns --- sentry/src/db/campaign.rs | 54 +++++++++++++++++++++++++++++++-------- 1 file changed, 43 insertions(+), 11 deletions(-) diff --git a/sentry/src/db/campaign.rs b/sentry/src/db/campaign.rs index 9c5287a20..ea24ec6f7 100644 --- a/sentry/src/db/campaign.rs +++ b/sentry/src/db/campaign.rs @@ -155,6 +155,11 @@ mod campaign_remaining { &self, campaigns: &[CampaignId], ) -> Result, RedisError> { + // `MGET` fails on empty keys + if campaigns.is_empty() { + return Ok(vec![]); + } + let keys: Vec = campaigns .iter() .map(|campaign| Self::get_key(*campaign)) @@ -314,6 +319,17 @@ mod campaign_remaining { let redis = TESTS_POOL.get().await.expect("Should return Object"); let campaign_remaining = CampaignRemaining::new(redis.connection.clone()); + // get multiple with empty campaigns slice + // `MGET` throws error on an empty keys argument + assert!( + campaign_remaining + .get_multiple(&[]) + .await + .expect("Should get multiple") + .is_empty(), + "Should return an empty result" + ); + let campaigns = (CampaignId::new(), CampaignId::new(), CampaignId::new()); // set initial amounts @@ -323,23 +339,39 @@ mod campaign_remaining { .await .expect("Should set value in redis")); - assert!(campaign_remaining - .set_initial(campaigns.1, UnifiedNum::from(200)) - .await - .expect("Should set value in redis")); + assert!(campaign_remaining + .set_initial(campaigns.1, UnifiedNum::from(200)) + .await + .expect("Should set value in redis")); - assert!(campaign_remaining - .set_initial(campaigns.2, UnifiedNum::from(300)) - .await - .expect("Should set value in redis")); + assert!(campaign_remaining + .set_initial(campaigns.2, UnifiedNum::from(300)) + .await + .expect("Should set value in redis")); } // set campaigns.1 to negative value, should return `0` because of `max(value, 0)` - assert_eq!(-300_i64, campaign_remaining.decrease_by(campaigns.1, UnifiedNum::from(500)).await.expect("Should decrease remaining")); + assert_eq!( + -300_i64, + campaign_remaining + .decrease_by(campaigns.1, UnifiedNum::from(500)) + .await + .expect("Should decrease remaining") + ); - let multiple = campaign_remaining.get_multiple(&[campaigns.0, campaigns.1, campaigns.2]).await.expect("Should get multiple"); + let multiple = campaign_remaining + .get_multiple(&[campaigns.0, campaigns.1, campaigns.2]) + .await + .expect("Should get multiple"); - assert_eq!(vec![UnifiedNum::from(100), UnifiedNum::from(0), UnifiedNum::from(300)], multiple); + assert_eq!( + vec![ + UnifiedNum::from(100), + UnifiedNum::from(0), + UnifiedNum::from(300) + ], + multiple + ); } } } From 9a94c5f1fad1e8577c027c7c2ba7528b381c3db1 Mon Sep 17 00:00:00 2001 From: Lachezar Lechev Date: Mon, 9 Aug 2021 11:15:27 +0300 Subject: [PATCH 08/10] sentry - routes - campaign create/modify - more checks & tests --- sentry/src/routes/campaign.rs | 310 +++++++++++++++++++++++++++++----- 1 file changed, 269 insertions(+), 41 deletions(-) diff --git a/sentry/src/routes/campaign.rs b/sentry/src/routes/campaign.rs index 6d57607b0..7fd2b9b50 100644 --- a/sentry/src/routes/campaign.rs +++ b/sentry/src/routes/campaign.rs @@ -1,4 +1,13 @@ -use crate::{Application, Auth, ResponseError, Session, access::{self, check_access}, db::{CampaignRemaining, DbPool, RedisError, accounting::get_accounting_spent, campaign::{get_campaigns_by_channel, insert_campaign, update_campaign}, spendable::fetch_spendable}, success_response}; +use crate::{ + access::{self, check_access}, + db::{ + accounting::get_accounting_spent, + campaign::{get_campaigns_by_channel, insert_campaign, update_campaign}, + spendable::fetch_spendable, + CampaignRemaining, DbPool, RedisError, + }, + success_response, Application, Auth, ResponseError, Session, +}; use chrono::Utc; use deadpool_postgres::PoolError; use hyper::{Body, Request, Response}; @@ -69,37 +78,55 @@ pub async fn create_campaign( let error_response = ResponseError::BadRequest("err occurred; please try again later".to_string()); - let accounting_spent = - get_accounting_spent(app.pool.clone(), &campaign.creator, &campaign.channel.id()).await?; + let total_remaining = + { + let accounting_spent = + get_accounting_spent(app.pool.clone(), &campaign.creator, &campaign.channel.id()) + .await?; - let latest_spendable = - fetch_spendable(app.pool.clone(), &campaign.creator, &campaign.channel.id()) - .await? - .ok_or(ResponseError::BadRequest( - "No spendable amount found for the Campaign creator".to_string(), - ))?; - let total_deposited = latest_spendable.deposit.total; + let latest_spendable = + fetch_spendable(app.pool.clone(), &campaign.creator, &campaign.channel.id()) + .await? + .ok_or(ResponseError::BadRequest( + "No spendable amount found for the Campaign creator".to_string(), + ))?; + // Gets the latest Spendable for this (spender, channelId) pair + let total_deposited = latest_spendable.deposit.total; - let remaining_for_channel = - total_deposited - .checked_sub(&accounting_spent) - .ok_or(ResponseError::FailedValidation( - "No more budget remaining".to_string(), - ))?; + total_deposited.checked_sub(&accounting_spent).ok_or( + ResponseError::FailedValidation("No more budget remaining".to_string()), + )? + }; - if campaign.budget > remaining_for_channel { + let channel_campaigns = get_campaigns_by_channel(&app.pool, &campaign.channel.id()) + .await? + .iter() + .map(|c| c.id) + .collect::>(); + + let campaigns_remaining_sum = app + .campaign_remaining + .get_multiple(&channel_campaigns) + .await? + .iter() + .sum::>() + .ok_or(Error::Calculation)? + // DO NOT FORGET to add the Campaign being created right now! + .checked_add(&campaign.budget) + .ok_or(Error::Calculation)?; + + if !(campaigns_remaining_sum <= total_remaining) || campaign.budget > total_remaining { return Err(ResponseError::BadRequest( - "Not enough deposit left for the new campaign budget".to_string(), + "Not enough deposit left for the new campaign's budget".to_string(), )); } // If the campaign is being created, the amount spent is 0, therefore remaining = budget - let remaining_set = CampaignRemaining::new(app.redis.clone()).set_initial(campaign.id, campaign.budget) + let remaining_set = CampaignRemaining::new(app.redis.clone()) + .set_initial(campaign.id, campaign.budget) .await .map_err(|_| { - ResponseError::BadRequest( - "Couldn't set remaining while creating campaign".to_string(), - ) + ResponseError::BadRequest("Couldn't set remaining while creating campaign".to_string()) })?; // If for some reason the randomly generated `CampaignId` exists in Redis @@ -107,7 +134,7 @@ pub async fn create_campaign( if !remaining_set { return Err(ResponseError::Conflict( "The generated CampaignId already exists, please repeat the request".to_string(), - )) + )); } // insert Campaign @@ -152,12 +179,10 @@ pub mod update_campaign { let modify_campaign_fields = serde_json::from_slice::(&body) .map_err(|e| ResponseError::FailedValidation(e.to_string()))?; - let campaign_remaining = CampaignRemaining::new(app.redis.clone()); - // modify Campaign let modified_campaign = modify_campaign( &app.pool, - campaign_remaining, + &app.campaign_remaining, campaign_being_mutated, modify_campaign_fields, ) @@ -169,7 +194,7 @@ pub mod update_campaign { pub async fn modify_campaign( pool: &DbPool, - campaign_remaining: CampaignRemaining, + campaign_remaining: &CampaignRemaining, campaign: Campaign, modify_campaign: ModifyCampaign, ) -> Result { @@ -178,7 +203,7 @@ pub mod update_campaign { // *NOTE*: To close a campaign set campaignBudget to campaignSpent so that spendable == 0 let delta_budget = if let Some(new_budget) = modify_campaign.budget { - get_delta_budget(&campaign_remaining, &campaign, new_budget).await? + get_delta_budget(campaign_remaining, &campaign, new_budget).await? } else { None }; @@ -229,7 +254,9 @@ pub mod update_campaign { .ok_or(Error::Calculation)?; if !(new_campaigns_remaining <= total_remaining) { - return Err(Error::CampaignNotModified); + return Err(Error::NewBudget( + "Not enough deposit left for the campaign's new budget".to_string(), + )); } // there is a chance that the new remaining will be negative even when increasing the budget @@ -402,25 +429,226 @@ async fn process_events( #[cfg(test)] mod test { - use super::*; + use super::{update_campaign::modify_campaign, *}; use crate::{ - db::{redis_pool::TESTS_POOL, tests_postgres::DATABASE_POOL}, + db::{accounting::insert_accounting, spendable::insert_spendable}, + test_util::setup_dummy_app, + }; + use hyper::StatusCode; + use primitives::{ + sentry::accounting::{Balances, CheckedState}, + spender::{Deposit, Spendable}, + util::tests::prep_db::DUMMY_CAMPAIGN, + ValidatorId, }; - use adapter::DummyAdapter; - use primitives::util::tests::prep_db::DUMMY_CAMPAIGN; #[tokio::test] /// Test single campaign creation and modification // & /// Test with multiple campaigns (because of Budget) a modification of campaign async fn create_and_modify_with_multiple_campaigns() { - todo!() - // let redis = TESTS_POOL.get().await.expect("Should get redis"); - // let postgres = DATABASE_POOL.get().await.expect("Should get postgres"); - // let campaign = DUMMY_CAMPAIGN.clone(); - - // let app = Application::new() - - + let app = setup_dummy_app().await; + + let build_request = |create_campaign: CreateCampaign| -> Request { + let auth = Auth { + era: 0, + uid: ValidatorId::from(create_campaign.creator), + }; + + let body = + Body::from(serde_json::to_string(&create_campaign).expect("Should serialize")); + + Request::builder() + .extension(auth) + .body(body) + .expect("Should build Request") + }; + + let campaign: Campaign = { + // erases the CampaignId for the CreateCampaign request + let mut create = CreateCampaign::from(DUMMY_CAMPAIGN.clone()); + // 500.00000000 + create.budget = UnifiedNum::from(50_000_000_000); + + let spendable = Spendable { + spender: create.creator, + channel: create.channel.clone(), + deposit: Deposit { + // a deposit equal to double the Campaign Budget + total: UnifiedNum::from(200_000_000_000), + still_on_create2: UnifiedNum::from(0), + }, + }; + assert!(insert_spendable(app.pool.clone(), &spendable) + .await + .expect("Should insert Spendable for Campaign creator")); + + let mut balances = Balances::::default(); + balances.add_spender(create.creator); + + // TODO: Replace this once https://github.com/AdExNetwork/adex-validator-stack-rust/pull/413 is merged + let _accounting = insert_accounting(app.pool.clone(), create.channel.clone(), balances) + .await + .expect("Should create Accounting"); + + let create_response = create_campaign(build_request(create), &app) + .await + .expect("Should create campaign"); + + assert_eq!(StatusCode::OK, create_response.status()); + let json = hyper::body::to_bytes(create_response.into_body()) + .await + .expect("Should get json"); + + let campaign: Campaign = + serde_json::from_slice(&json).expect("Should get new Campaign"); + + assert_ne!(DUMMY_CAMPAIGN.id, campaign.id); + + let campaign_remaining = CampaignRemaining::new(app.redis.clone()); + + let remaining = campaign_remaining + .get_remaining_opt(campaign.id) + .await + .expect("Should get remaining from redis") + .expect("There should be value for the Campaign"); + + assert_eq!( + UnifiedNum::from(50_000_000_000), + UnifiedNum::from(remaining.unsigned_abs()) + ); + campaign + }; + + // modify campaign + let modified = { + // 1000.00000000 + let new_budget = UnifiedNum::from(100_000_000_000); + let modify = ModifyCampaign { + budget: Some(new_budget.clone()), + validators: None, + title: Some("Updated title".to_string()), + pricing_bounds: None, + event_submission: None, + ad_units: None, + targeting_rules: None, + }; + + let modified_campaign = + modify_campaign(&app.pool, &app.campaign_remaining, campaign.clone(), modify) + .await + .expect("Should modify campaign"); + + assert_eq!(new_budget, modified_campaign.budget); + assert_eq!(Some("Updated title".to_string()), modified_campaign.title); + + modified_campaign + }; + + // we have 1000 left from our deposit, so we are using half of it + let _second_campaign = { + // erases the CampaignId for the CreateCampaign request + let mut create_second = CreateCampaign::from(DUMMY_CAMPAIGN.clone()); + // 500.00000000 + create_second.budget = UnifiedNum::from(50_000_000_000); + + let create_response = create_campaign(build_request(create_second), &app) + .await + .expect("Should create campaign"); + + assert_eq!(StatusCode::OK, create_response.status()); + let json = hyper::body::to_bytes(create_response.into_body()) + .await + .expect("Should get json"); + + let second_campaign: Campaign = + serde_json::from_slice(&json).expect("Should get new Campaign"); + + second_campaign + }; + + // No budget left for new campaigns + // remaining: 500 + // new campaign budget: 600 + { + // erases the CampaignId for the CreateCampaign request + let mut create = CreateCampaign::from(DUMMY_CAMPAIGN.clone()); + // 600.00000000 + create.budget = UnifiedNum::from(60_000_000_000); + + let create_err = create_campaign(build_request(create), &app) + .await + .expect_err("Should return Error response"); + + assert_eq!(ResponseError::BadRequest("Not enough deposit left for the new campaign's budget".to_string()), create_err); + } + + // modify first campaign, by lowering the budget from 1000 to 900 + let modified = { + let lower_budget = UnifiedNum::from(90_000_000_000); + let modify = ModifyCampaign { + budget: Some(lower_budget.clone()), + validators: None, + title: None, + pricing_bounds: None, + event_submission: None, + ad_units: None, + targeting_rules: None, + }; + + let modified_campaign = + modify_campaign(&app.pool, &app.campaign_remaining, modified, modify) + .await + .expect("Should modify campaign"); + + assert_eq!(lower_budget, modified_campaign.budget); + + modified_campaign + }; + + // Just enough budget to create this Campaign + // remaining: 600 + // new campaign budget: 600 + { + // erases the CampaignId for the CreateCampaign request + let mut create = CreateCampaign::from(DUMMY_CAMPAIGN.clone()); + // 600.00000000 + create.budget = UnifiedNum::from(60_000_000_000); + + let create_response = create_campaign(build_request(create), &app) + .await + .expect("Should return create campaign"); + + let json = hyper::body::to_bytes(create_response.into_body()) + .await + .expect("Should get json"); + + let _campaign: Campaign = + serde_json::from_slice(&json).expect("Should get new Campaign"); + } + + // Modify a campaign without enough budget + // remaining: 0 + // new campaign budget: 1100 + // current campaign budget: 900 + { + let new_budget = UnifiedNum::from(110_000_000_000); + let modify = ModifyCampaign { + budget: Some(new_budget), + validators: None, + title: None, + pricing_bounds: None, + event_submission: None, + ad_units: None, + targeting_rules: None, + }; + + let modify_err = + modify_campaign(&app.pool, &app.campaign_remaining, modified, modify) + .await + .expect_err("Should return Error response"); + + assert!(matches!(modify_err, Error::NewBudget(string) if string == "Not enough deposit left for the campaign's new budget")); + } } } From 5177217cd7d24ddf261a855983259dabf52311f5 Mon Sep 17 00:00:00 2001 From: Lachezar Lechev Date: Mon, 9 Aug 2021 12:46:50 +0300 Subject: [PATCH 09/10] sentry - db - make pub insert_accounting --- sentry/src/db/accounting.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/sentry/src/db/accounting.rs b/sentry/src/db/accounting.rs index ffc8e3db4..94fcae16e 100644 --- a/sentry/src/db/accounting.rs +++ b/sentry/src/db/accounting.rs @@ -38,9 +38,7 @@ pub async fn get_accounting_spent( Ok(row.get("spent")) } -// TODO This is still WIP -#[allow(dead_code)] -async fn insert_accounting( +pub async fn insert_accounting( pool: DbPool, channel: Channel, balances: Balances, From 75a5a5f63350f4a1991fa7c209329f1b8b142f1c Mon Sep 17 00:00:00 2001 From: Lachezar Lechev Date: Mon, 9 Aug 2021 15:46:53 +0300 Subject: [PATCH 10/10] sentry - routes - campaign - get_delta_budget - fix logic & naming --- sentry/src/routes/campaign.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/sentry/src/routes/campaign.rs b/sentry/src/routes/campaign.rs index 7fd2b9b50..b3db3abe1 100644 --- a/sentry/src/routes/campaign.rs +++ b/sentry/src/routes/campaign.rs @@ -330,6 +330,7 @@ pub mod update_campaign { .checked_sub(¤t_budget) .and_then(|delta_budget| old_remaining.checked_add(&delta_budget)) .ok_or(Error::Calculation)?; + // new remaining > old remaining let increase_by = new_remaining .checked_sub(&old_remaining) .ok_or(Error::Calculation)?; @@ -337,13 +338,14 @@ pub mod update_campaign { DeltaBudget::Increase(increase_by) } DeltaBudget::Decrease(()) => { - // delta budget = New budget - Old budget ( the difference between the new and old when New > Old) + // delta budget = Old budget - New budget ( the difference between the new and old when New < Old) let new_remaining = ¤t_budget .checked_sub(&new_budget) - .and_then(|delta_budget| old_remaining.checked_add(&delta_budget)) + .and_then(|delta_budget| old_remaining.checked_sub(&delta_budget)) .ok_or(Error::Calculation)?; - let decrease_by = new_remaining - .checked_sub(&old_remaining) + // old remaining > new remaining + let decrease_by = old_remaining + .checked_sub(&new_remaining) .ok_or(Error::Calculation)?; DeltaBudget::Decrease(decrease_by)