From f44959e8ab0d752e124d9e14abe434c73c80780a Mon Sep 17 00:00:00 2001 From: Franco Testagrossa Date: Mon, 6 May 2024 19:03:40 +0200 Subject: [PATCH] Refactor bounty to monolithic approach --- Cargo.lock | 85 ++++++++++++++++++++++++++++++++ backend/Cargo.toml | 1 + backend/backend.did | 4 +- backend/src/bounty/api/accept.rs | 63 +++++++++++++++++------ backend/src/bounty/api/claim.rs | 80 +++++++++++++++++++++++------- backend/src/bounty/api/init.rs | 9 ++-- backend/src/bounty/api/state.rs | 36 ++++++++++---- backend/src/lib.rs | 11 +++-- 8 files changed, 236 insertions(+), 53 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index dd7f8be..3fc798f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -46,6 +46,7 @@ version = "0.1.0" dependencies = [ "async-trait", "candid", + "derive_builder", "futures", "ic-cdk", "ic-cdk-macros", @@ -172,12 +173,78 @@ dependencies = [ "typenum", ] +[[package]] +name = "darling" +version = "0.20.8" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "54e36fcd13ed84ffdfda6f5be89b31287cbb80c439841fe69e04841435464391" +dependencies = [ + "darling_core", + "darling_macro", +] + +[[package]] +name = "darling_core" +version = "0.20.8" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9c2cf1c23a687a1feeb728783b993c4e1ad83d99f351801977dd809b48d0a70f" +dependencies = [ + "fnv", + "ident_case", + "proc-macro2", + "quote", + "strsim", + "syn 2.0.60", +] + +[[package]] +name = "darling_macro" +version = "0.20.8" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a668eda54683121533a393014d8692171709ff57a7d61f187b6e782719f8933f" +dependencies = [ + "darling_core", + "quote", + "syn 2.0.60", +] + [[package]] name = "data-encoding" version = "2.6.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e8566979429cf69b49a5c740c60791108e86440e8be149bbea4fe54d2c32d6e2" +[[package]] +name = "derive_builder" +version = "0.20.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0350b5cb0331628a5916d6c5c0b72e97393b8b6b03b47a9284f4e7f5a405ffd7" +dependencies = [ + "derive_builder_macro", +] + +[[package]] +name = "derive_builder_core" +version = "0.20.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d48cda787f839151732d396ac69e3473923d54312c070ee21e9effcaa8ca0b1d" +dependencies = [ + "darling", + "proc-macro2", + "quote", + "syn 2.0.60", +] + +[[package]] +name = "derive_builder_macro" +version = "0.20.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "206868b8242f27cecce124c19fd88157fbd0dd334df2587f36417bafbc85097b" +dependencies = [ + "derive_builder_core", + "syn 2.0.60", +] + [[package]] name = "digest" version = "0.10.7" @@ -194,6 +261,12 @@ version = "1.11.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a47c1c47d2f5964e29c61246e81db715514cd532db6b5116a25ea3c03d6780a2" +[[package]] +name = "fnv" +version = "1.0.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3f9eec918d3f24069decb9af1554cad7c880e2da24a9afd88aca000531ab82c1" + [[package]] name = "futures" version = "0.3.30" @@ -360,6 +433,12 @@ dependencies = [ "thiserror", ] +[[package]] +name = "ident_case" +version = "1.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b9e0384b61958566e926dc50660321d12159025e767c18e043daf26b70104c39" + [[package]] name = "itoa" version = "1.0.11" @@ -601,6 +680,12 @@ dependencies = [ "winapi", ] +[[package]] +name = "strsim" +version = "0.10.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "73473c0e59e6d5812c5dfe2a064a6444949f089e20eec9a2e5506596494e4623" + [[package]] name = "syn" version = "1.0.109" diff --git a/backend/Cargo.toml b/backend/Cargo.toml index 73d5d03..af4e1a8 100644 --- a/backend/Cargo.toml +++ b/backend/Cargo.toml @@ -22,3 +22,4 @@ num-traits = "0.2.18" futures = "0.3.30" async-trait = "0.1.8" regex = "1.5.4" +derive_builder = "0.20.0" diff --git a/backend/backend.did b/backend/backend.did index 4c7655f..469d5e4 100644 --- a/backend/backend.did +++ b/backend/backend.did @@ -50,7 +50,7 @@ type DepositErr = variant { TransferFailure : record { reason : text }; }; -service : (authority: principal, github_issue_id: int32) -> { +service : (authority: principal) -> { // GitHub Service "get_issue": (GithubToken) -> (Issue); "get_fixed_by": (GithubToken) -> (text); @@ -58,7 +58,7 @@ service : (authority: principal, github_issue_id: int32) -> { "get_merged_details": (GithubToken) -> (PrDetailsResponse); // Bounty Service "healthcheck": () -> (text); - "accept": (Contributor, github_pr_id: int32) -> (); + "accept": (Contributor, github_issue_id: int32, github_pr_id: int32) -> (); "deposit": () -> (DepositReceipt); } diff --git a/backend/src/bounty/api/accept.rs b/backend/src/bounty/api/accept.rs index 8d9e6d6..48123c0 100644 --- a/backend/src/bounty/api/accept.rs +++ b/backend/src/bounty/api/accept.rs @@ -1,19 +1,33 @@ -use super::state::{Contributor, BOUNTY_STATE}; +use super::state::{Contributor, PullRequest, BOUNTY_STATE}; -pub fn accept_impl(contributor: Contributor, github_pr_id: i32) -> () { - // FIXME: check contributor is the owner of the PR. +pub fn accept_impl(contributor: Contributor, github_issue_id: i32, github_pr_id: i32) -> () { BOUNTY_STATE.with(|state| { if let Some(ref mut bounty_canister) = *state.borrow_mut() { - if bounty_canister.interested_contributors.contains_key(&github_pr_id) { - // do not update if the key is present. - // > The contributor is suppose to be the PR owner and the principal who called accept, - // > thus its fixed. - // FIXME: change response type to include a propper domain error. + let mut issue_exists = false; + let mut pr_exists = false; + + if let Some(ref mut issue) = bounty_canister.github_issues.get_mut(&github_issue_id) { + issue_exists = true; + if !issue.bounty.accepted_prs.contains_key(&github_pr_id) { + let pr = PullRequest { + id: github_pr_id, + contributor, + }; + issue.bounty.accepted_prs.insert(github_pr_id, pr); + pr_exists = true; + } + } + + if !issue_exists { + // FIXME: change response type to include a proper domain error. + // The response should be a Result type (Either). + panic!("Can't accept an issue which does not exist."); + } + + if !pr_exists { + // FIXME: change response type to include a proper domain error. // The response should be a Result type (Either). panic!("Can't accept twice"); - } else { - // Add the contributor to the interested contributors list. - bounty_canister.interested_contributors.insert(github_pr_id, contributor); } } }); @@ -30,15 +44,26 @@ mod test_accept { let authority = Principal::from_text("t2y5w-qp34w-qixaj-s67wp-syrei-5yqse-xbed6-z5nsd-fszmf-izgt2-lqe") .unwrap(); - init_impl(authority, 123); + init_impl(authority); + let github_issue_id = 123; BOUNTY_STATE.with(|state| { let bounty_canister = state.borrow(); if let Some(ref bounty_canister) = *bounty_canister { - assert_eq!(bounty_canister.interested_contributors.len(), 0); + assert_eq!( + bounty_canister + .github_issues + .get(&github_issue_id) + .unwrap() + .bounty + .accepted_prs + .len(), + 0 + ); } else { panic!("Bounty canister state not initialized"); } }); + let contributor = Principal::from_text("t2y5w-qp34w-qixaj-s67wp-syrei-5yqse-xbed6-z5nsd-fszmf-izgt2-lqe") .unwrap(); @@ -48,12 +73,22 @@ mod test_accept { address: contributor, crypto_address: "contributor_address".to_string(), }, + github_issue_id, github_pr_id, ); BOUNTY_STATE.with(|state| { let bounty_canister = state.borrow(); if let Some(ref bounty_canister) = *bounty_canister { - assert_eq!(bounty_canister.interested_contributors.len(), 1); + assert_eq!( + bounty_canister + .github_issues + .get(&github_issue_id) + .unwrap() + .bounty + .accepted_prs + .len(), + 1 + ); } else { panic!("Bounty canister state not initialized"); } diff --git a/backend/src/bounty/api/claim.rs b/backend/src/bounty/api/claim.rs index 9c490aa..2b3f3a0 100644 --- a/backend/src/bounty/api/claim.rs +++ b/backend/src/bounty/api/claim.rs @@ -1,8 +1,10 @@ -use crate::bounty::api::state::{BOUNTY_STATE, Contributor}; +use crate::bounty::api::state::{Contributor, BOUNTY_STATE}; use candid::{CandidType, Principal}; use serde::{Deserialize, Serialize}; -use crate::provider::github::api::{get_issue::IssueResponse, get_merged_details::PrDetailsResponse}; +use crate::provider::github::api::{ + get_issue::IssueResponse, get_merged_details::PrDetailsResponse, +}; use crate::provider::github::client::IGithubClient; #[derive(Debug, Serialize, Deserialize, CandidType)] @@ -21,10 +23,18 @@ pub struct GithubClientMock { #[cfg(test)] #[async_trait::async_trait] impl IGithubClient for GithubClientMock { - async fn get_issue(&self, issue_nbr: i32) -> IssueResponse { todo!() } - async fn get_fixed_by(&self, issue_nbr: i32) -> String { todo!() } - async fn get_is_merged(&self, pr_nbr: i32) -> String { todo!() } - async fn get_merged_details(&self, pr_nbr: i32) -> PrDetailsResponse { todo!() } + async fn get_issue(&self, issue_nbr: i32) -> IssueResponse { + todo!() + } + async fn get_fixed_by(&self, issue_nbr: i32) -> String { + todo!() + } + async fn get_is_merged(&self, pr_nbr: i32) -> String { + todo!() + } + async fn get_merged_details(&self, pr_nbr: i32) -> PrDetailsResponse { + todo!() + } } // FIXME: remove this annotation after finishing draft impl. @@ -39,14 +49,18 @@ pub async fn claim_impl( match state.borrow().as_ref() { Some(bounty_state) => { // Access the interested_contributors HashMap from the BountyState - bounty_state.interested_contributors.get(&github_pr_id).cloned() + return bounty_state + .github_issues + .get(&github_pr_id) + .and_then(|issue| issue.bounty.accepted_prs.get(&github_pr_id)) + .map(|pr| pr.contributor.clone()) } - None => panic!("Bounty canister state not initialized") + None => panic!("Bounty canister state not initialized"), } }); match contributor_opt { - None => Some(ClaimError::PRNotAccepted{github_pr_id}) - , Some(contributor) => { + None => Some(ClaimError::PRNotAccepted { github_pr_id }), + Some(contributor) => { let issue = github_client.get_issue(github_issue_id).await; todo!() } @@ -63,12 +77,18 @@ mod test_claim { use super::{claim_impl, ClaimError, GithubClientMock}; - fn accept_contributor(principal: &str, crypto_address: &str, github_pr_id: i32) { + fn accept_contributor( + principal: &str, + crypto_address: &str, + github_issue_id: i32, + github_pr_id: i32, + ) { accept_impl( Contributor { address: Principal::from_text(principal).unwrap(), crypto_address: crypto_address.to_string(), }, + github_issue_id, github_pr_id, ); } @@ -79,14 +99,31 @@ mod test_claim { let authority = Principal::from_text("rdmx6-jaaaa-aaaaa-aaadq-cai").unwrap(); - init_impl(authority, github_issue_id); + init_impl(authority); - accept_contributor("mxzaz-hqaaa-aaaar-qaada-cai", "contributor_address_1", 1); - accept_contributor("n5wcd-faaaa-aaaar-qaaea-cai", "contributor_address_2", 2); + accept_contributor( + "mxzaz-hqaaa-aaaar-qaada-cai", + "contributor_address_1", + github_issue_id, + 1, + ); + accept_contributor( + "n5wcd-faaaa-aaaar-qaaea-cai", + "contributor_address_2", + github_issue_id, + 2, + ); - let github_client = GithubClientMock{principal: authority}; + let github_client = GithubClientMock { + principal: authority, + }; - let result = block_on(claim_impl(&github_client, github_issue_id, 2, "GithubToken")); + let result = block_on(claim_impl( + &github_client, + github_issue_id, + 2, + "GithubToken", + )); match result { None => assert!(true), @@ -99,7 +136,16 @@ mod test_claim { BOUNTY_STATE.with(|state| { let bounty_canister = state.borrow(); if let Some(ref bounty_canister) = *bounty_canister { - assert_eq!(bounty_canister.winner.unwrap(), 2); + assert_eq!( + bounty_canister + .github_issues + .get(&github_issue_id) + .unwrap() + .bounty + .winner + .unwrap(), + 2 + ); } else { panic!("Bounty canister state not initialized"); } diff --git a/backend/src/bounty/api/init.rs b/backend/src/bounty/api/init.rs index 585b5b3..732caf9 100644 --- a/backend/src/bounty/api/init.rs +++ b/backend/src/bounty/api/init.rs @@ -3,14 +3,11 @@ use std::collections::HashMap; use super::state::{BountyState, BOUNTY_STATE}; use candid::Principal; -pub fn init_impl(authority: Principal, github_issue_id: i32) -> () { +pub fn init_impl(authority: Principal) -> () { BOUNTY_STATE.with(|state| { *state.borrow_mut() = Some(BountyState { authority, - github_issue_id, - interested_contributors: HashMap::new(), - claimed: false, - winner: None + github_issues: HashMap::new(), }); }); } @@ -30,7 +27,7 @@ mod test_init { Principal::from_text("t2y5w-qp34w-qixaj-s67wp-syrei-5yqse-xbed6-z5nsd-fszmf-izgt2-lqe") .unwrap(); - init_impl(authority, 123); + init_impl(authority); BOUNTY_STATE.with(|state| { let bounty_canister = state.borrow(); assert!(bounty_canister.is_some()); diff --git a/backend/src/bounty/api/state.rs b/backend/src/bounty/api/state.rs index 867d3b1..bc2ce88 100644 --- a/backend/src/bounty/api/state.rs +++ b/backend/src/bounty/api/state.rs @@ -6,21 +6,37 @@ use serde::{Deserialize, Serialize}; type IssueId = i32; type PullRequestId = i32; -#[derive(Debug, Serialize, Deserialize, CandidType)] -pub struct BountyState { - pub authority: Principal, - pub github_issue_id: IssueId, - pub interested_contributors: HashMap, - pub claimed: bool, - pub winner: Option, -} - -#[derive(Debug, Serialize, Deserialize, CandidType, Clone)] +#[derive(Debug, Serialize, Deserialize, CandidType, Clone, Builder)] pub struct Contributor { pub address: Principal, pub crypto_address: String, } +#[derive(Debug, Serialize, Deserialize, CandidType, Clone, Builder)] +pub struct PullRequest { + pub id: PullRequestId, + pub contributor: Contributor, +} + +#[derive(Debug, Serialize, Deserialize, CandidType, Clone, Builder)] +pub struct Bounty { + pub amount: i32, + pub winner: Option, + pub accepted_prs: HashMap, +} + +#[derive(Debug, Serialize, Deserialize, CandidType, Clone, Builder)] +pub struct Issue { + pub id: IssueId, + pub maintainer: Contributor, + pub bounty: Bounty, +} + +#[derive(Debug, Serialize, Deserialize, CandidType)] +pub struct BountyState { + pub authority: Principal, + pub github_issues: HashMap, +} // Define thread-local storage for the bounty canister state // WASM is single-threaded by nature. [RefCell] and [thread_local!] are used despite being not totally safe primitives. // This is to ensure that the canister state can be used throughout. diff --git a/backend/src/lib.rs b/backend/src/lib.rs index 4aee131..8d4043d 100644 --- a/backend/src/lib.rs +++ b/backend/src/lib.rs @@ -1,3 +1,6 @@ +#[macro_use] +extern crate derive_builder; + use candid::Principal; // GITHUB SERVICE @@ -89,13 +92,13 @@ async fn get_merged_details(github_token: String) -> PrDetailsResponse { // BOUNTY SERVICE #[ic_cdk::init] -fn init(authority: Principal, github_issue_id: i32) -> () { - init_impl(authority, github_issue_id); +fn init(authority: Principal) -> () { + init_impl(authority); } #[ic_cdk::update] -fn accept(contributor: Contributor, github_pr_id: i32) -> () { - accept_impl(contributor, github_pr_id); +fn accept(contributor: Contributor, github_issue_id: i32, github_pr_id: i32) -> () { + accept_impl(contributor, github_issue_id, github_pr_id); } #[ic_cdk::update]