lfglabs-dev / api.starknet.quest

starknet.quest rust backend
7 stars 28 forks source link

Fix task ids #274

Closed Marchand-Nicolas closed 1 month ago

Marchand-Nicolas commented 1 month ago

In each task creation route, there are these lines:

    let last_doc = &collection.find_one(last_id_filter, options).await.unwrap();

    let mut next_id = 1;
    if let Some(doc) = last_doc {
        let last_id = doc.id;
        next_id = last_id + 1;
    }

e.g. in src\endpoints\admin\discord\create_discord.rs

The problem is that we often send task creation requests simultaneously and all of the created tasks get the same id, as interacting with the db is too slow. I suggest you to make all this code a function in src\utils.rs, that checks in the db (as be currently do) and at the same time also checks in the shared state (i.e. the state variable you see in almost every file), and takes the greatest id returned by both. That way we update a (new) last_task_id variable very quickly in the state, but still read from db in the case data from db is newer (e.g. when restarting the server). And that's why I suggest you to write this in a new function, as it will start getting a pretty big block of code that would otherwise be repeated in every task creation route.

Ugo-X commented 1 month ago

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

I am a Full Stack blockchain Developer with expertise in Next.js, Nest.js, TypeScript, JavaScript, React, Node.js, Three.js, Solidity and rust. My journey with OnlyDust hackathons began at Edition 1, and I've since made 69 contributions across 14 projects. With my extensive experience on the OnlyDust platform (profile: https://app.onlydust.com/u/Ugo-X), I've honed my skills in delivering quality solutions under pressure.

How I plan on tackling this issue

I will define a function to solve the issue of duplicate task IDs during concurrent creation requests.

Steps:

Function Creation: I will create a new function named get_next_task_id in src/utils.rs. Database Check: This function will first perform the existing logic of checking the database for the last task ID using collection.find_one.

Shared State Check: Simultaneously (using concurrency primitives like Mutex), it will access the shared state variable (last_task_id) and store its value. ID Comparison: The function will then compare the retrieved values from the database and shared state. Return Highest ID: It will return the highest value among the two IDs (max(db_id, state_id)). State Update: After returning the ID, the function will update the shared state variable with the new value (incremented by 1).

ScottyDavies commented 1 month ago

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

I am a backend developer with years of experience

How I plan on tackling this issue

To address the issue of simultaneous task creation requests resulting in duplicate IDs, I would create a function in src/utils.rs that fetches the greatest ID from both the database and the shared state. This function will quickly update the last_task_id variable in the state while still reading from the database when necessary, such as after server restarts. This approach will help avoid repeating the same block of code in every task creation route.

akintewe commented 1 month ago

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

I have expertise working with concurrent systems and database operations as a mobile developer with a CS degree. Although I've worked mostly on mobile development, I've also used Rust for high-performance applications on backend services. My knowledge of racial circumstances and systems programming experience will come in handy while resolving this task ID issue. The key to resolving this issue is my familiarity with shared state management and database interaction optimisation.

How I plan on tackling this issue

To start, I would add a new function to src/utils.rs to manage the creation of task IDs. The parameters for this function would be the shared state and the database connection. In addition to checking the shared state for the last known ID, it would query the database for the last used ID. In order to guarantee uniqueness, the function would then return the maximum of these two numbers plus one. In order to ensure thread safety, I would add a last task id field to the shared state using an atomic integer. The new function would create a new ID and then update this field. Lastly, I would call this new method instead of the current ID generating code in each task creation route. To make sure IDs stay, I would carefully test the solution under scenarios of concurrent task creation.

mubarak23 commented 1 month ago

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

I am a Experience Cairo smart contract developer with experience working on projects such as Just Art Peace, Dojo, Kart, TBA, and Shinigami. Before transitioning to Cairo development, I was a backend developer specializing in Rust.

How I plan on tackling this issue

Function Definition: I'll implement a new function called get_next_task_id in src/utils.rs.

Database Lookup: The function will begin by querying the database for the most recent task ID using collection.find_one.

Shared State Access: Concurrently, it will access the shared state variable last_task_id through concurrency tools like Mutex to retrieve its value.

ID Comparison: After fetching the IDs from both the database and the shared state, the function will compare them.

Return Highest ID: The higher of the two IDs will be returned (max(db_id, state_id)).

State Update: Finally, the shared state will be updated with the next task ID by incrementing the returned value by 1.

mubarak23 commented 1 month ago

@Marchand-Nicolas does this mean, every where we are creating a QuestTaskDocument, we will call the get_next_task_id function to get the next id

Marchand-Nicolas commented 1 month ago

@Marchand-Nicolas does this mean, every where we are creating a QuestTaskDocument, we will call the get_next_task_id function to get the next id

Exactly!

akintewe commented 1 month ago

I would like to be assigned this task

mubarak23 commented 1 month ago

@Marchand-Nicolas

have you experience this kind of error when you build

error[E0277]: the trait bound `fn(axum::extract::State<Arc<AppState>>, Extension<std::string::String>, axum::Json<create_balance::CreateBalance>) -> impl futures::Future<Output = impl IntoResponse> {create_balance::handler}: Handler<_, _, _>` is not satisfied
   --> src/endpoints/admin/balance/create_balance.rs:29:14
    |
28  | #[route(post, "/admin/tasks/balance/create", auth_middleware)]
    | -------------------------------------------------------------- required by a bound introduced by this call
29  | pub async fn handler(
    |              ^^^^^^^ the trait `Handler<_, _, _>` is not implemented for fn item `fn(State<Arc<AppState>>, Extension<String>, Json<CreateBalance>) -> impl Future<Output = impl IntoResponse> {handler}`
    |
    = help: the following other types implement trait `Handler<T, S, B>`:
              <Layered<L, H, T, S, B, B2> as Handler<T, S, B2>>
              <MethodRouter<S, B> as Handler<(), S, B>>
note: required by a bound in `post`
   --> /Users/macbookpro/.cargo/registry/src/index.crates.io-6f17d22bba15001f/axum-0.6.20/src/routing/method_routing.rs:407:1
407 | top_level_handler_fn!(post, POST);
    | ^^^^^^^^^^^^^^^^^^^^^^----^^^^^^^
    | |                     |
    | |                     required by a bound in this function
    | required by this bound in `post`
    = note: this error originates in the macro `top_level_handler_fn` (in Nightly builds, run with -Z macro-backtrace for more info)