0xPolygonMiden / miden-client

Client library that facilitates interaction with the Miden rollup
MIT License
25 stars 22 forks source link

Database ORM #19

Open frisitano opened 7 months ago

frisitano commented 7 months ago

Should we consider the introduction of an ORM with native support for migrations to manage our data model. Some options include:

https://github.com/diesel-rs/diesel https://github.com/SeaQL/sea-orm

igamigo commented 7 months ago

We've been investigating Diesel, here is what it looks like so far: https://github.com/lambdaclass/miden-client/pull/5/files There is a decent amount of boilerplate but in general code seems to look cleaner and it allows us to get rid of migrations and some typing workarounds that we had to do since we are not saving entire objects (and related objects as well). We have not checked out SeaQL yet.

juan518munoz commented 6 months ago

I've been working with both alternatives to see how easy is to implement each, how much does the codebase need to change and what dependencies each ORM relies on.

Here's the diesel and seaORM (draft) pull requests, both have minimal implementations of the ORM, they ~have only what's needed to make the test insert_same_account_twice_fails() work as intended~ can be tested with the following commands:

Create an account:

General takeaways

Both ORMs rely on CLIs to be set up for the first time, migrations can be done with said CLIs or directly through code. Integration is rather tedious than difficult, I believe that the sooner we change to any of the ORMs, the easier it'll be.

image

source

About Diesel

Insert example:

    pub fn insert_account(&mut self, account: &Account) -> Result<(), StoreError> {
        use schema::accounts;

        let account = NewAccount::from_account(account).unwrap();
        diesel::insert_into(accounts::table)
            .values(account)
            .returning(Accounts::as_returning())
            .get_result(&mut self.db)
            .map_err(StoreError::QueryError)?;

        Ok(())
    }

Looks simple, but behind it there's some boiler plate, an Account can't be inserted directly into the database, it needs to be converted to a Insertable type:

#[derive(Insertable)]
#[diesel(table_name = schema::accounts)]
pub struct NewAccount {
    pub id: i64,
    pub code_root: Vec<u8>,
    pub storage_root: Vec<u8>,
    pub vault_root: Vec<u8>,
    pub nonce: i64,
    pub committed: bool,
}

impl NewAccount {
    pub fn from_account(account: &Account) -> Result<NewAccount, ()> {
        let id: u64 = account.id().into();
        Ok(NewAccount {
            id: id as i64,
            code_root: serde_json::to_string(&account.code().root())
                .unwrap()
                .into_bytes(),
            storage_root: serde_json::to_string(&account.storage().root())
                .unwrap()
                .into_bytes(),
            vault_root: serde_json::to_string(&account.vault().commitment())
                .unwrap()
                .into_bytes(),
            nonce: account.nonce().inner() as i64,
            committed: account.is_on_chain(),
        })
    }
}

This could be removed if Account derived Insertable, but being an expern type it's not possible to achieve.

Select example:

    pub fn get_accounts(&mut self) -> Result<Vec<AccountStub>, StoreError> {
        use schema::accounts::dsl::*;

        Ok(accounts
            .select(Accounts::as_select())
            .load(&mut self.db)
            .unwrap() // TODO: handle unwrap
            .iter()
            .map(|a| a.to_account_stub().unwrap()) // TODO: handle unwrap
            .collect())
    }

There's also some other boilerplate that's generated by the CLI, schema.rs.

Diesel doesn't need any other crates but itself and diesel_migrations to run migrations through code.

About SeaORM

Insert example:

    pub async fn insert_account(
        tx: &DatabaseTransaction,
        account: &Account,
    ) -> Result<(), StoreError> {
        let id: u64 = account.id().into();
        let account = accounts::ActiveModel {
            id: Set(id as i64),
            code_root: Set(serde_json::to_string(&account.code().root())
                .unwrap()
                .into_bytes()),
            storage_root: Set(serde_json::to_string(&account.storage().root())
                .unwrap()
                .into_bytes()),
            vault_root: Set(serde_json::to_string(&account.vault().commitment())
                .unwrap()
                .into_bytes()),
            nonce: Set(account.nonce().inner() as i64),
            committed: Set(account.is_on_chain()),
        };

        let _account = account.insert(tx).await.map_err(StoreError::QueryError)?;

        Ok(())
    }

Select example:

    pub async fn get_accounts(&self) -> Result<Vec<AccountStub>, StoreError> {
        Ok(accounts::Entity::find()
            .all(&self.db)
            .await
            .unwrap()
            .iter()
            .map(|account| {
                AccountStub::new(
                    (account.id as u64).try_into().unwrap(),
                    (account.nonce as u64).into(),
                    serde_json::from_str(&String::from_utf8(account.vault_root.clone()).unwrap())
                        .unwrap(),
                    serde_json::from_str(&String::from_utf8(account.storage_root.clone()).unwrap())
                        .unwrap(),
                    serde_json::from_str(&String::from_utf8(account.code_root.clone()).unwrap())
                        .unwrap(),
                )
            })
            .collect())
    }

Like Diesel, there's some boiler plate behind this query, on entity/src/account.rs we have the code implementationt for accounts::ActiveModel, but in the case of Sea this code is generated automatically by it's CLI.

The downside of this ORM is that it needs to have two directories on the root of the project, migration, where the migration functions lives (manually implemented), and entity, a library to interact with the CLI. A way to fix this would be to split cli and store into two different proyect that live on the same workspace, and then have seaORMs requiered entity and migration directories inside store.

As for dependencies, seaORM works in an asynchronous way, it's currently implemented with tokio, but if we want to make the code no std compatible, we will need to change tokio for a no std solution.

Conclusion

Choose Diesel if project structure needs to remain as is, and async is not preferable, otherwise SeaORM is faster to integrate.

References

Getting Started with Diesel Guide to getting started with SeaORM: an ORM for Rust SeaORM Tutorials

bobbinth commented 6 months ago

Thank you for the great write up! Overall, if we go with ORM, seems like Diesel is the way to go - but not clear to me yet if ORM simplifies things. A couple of questions:

Insert example:

    pub fn insert_account(&mut self, account: &Account) -> Result<(), StoreError> {
        use schema::accounts;

        let account = NewAccount::from_account(account).unwrap();
        diesel::insert_into(accounts::table)
            .values(account)
            .returning(Accounts::as_returning())
            .get_result(&mut self.db)
            .map_err(StoreError::QueryError)?;

        Ok(())
    }

Does this insert the account and all of its sub-components, or just the account record? (I looked though the PR, but it wasn't clear to me how insertion of sub-components is handled).

Looks simple, but behind it there's some boiler plate, an Account can't be inserted directly into the database, it needs to be converted to a Insertable type:

In the PR there are two identical structs defined: Account and NewAccount. Would we not make the Account struct defined in src/store/models.rs Insertable and get rid of NewAccount?

juan518munoz commented 6 months ago

Does this insert the account and all of its sub-components, or just the account record? (I looked though the PR, but it wasn't clear to me how insertion of sub-components is handled).

Both branches are based on a previous version of the client, were insert_account was a function of insert_account_with_metadata, which then were renamed to insert_account_record and insert_account respectively. So in short, the example insertion functions shown is actually insert_account_record.

In the PR there are two identical structs defined: Account and NewAccount. Would we not make the Account struct defined in src/store/models.rs Insertable and get rid of NewAccount?

There are three structs with similar names, these are:

mFragaBA commented 5 months ago

Another option that's built on top of sqlx is rbatis: https://github.com/rbatis/rbatis There's an example repo you can check(https://github.com/rbatis/abs_admin)