Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: update seaorm to latest #4925

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Mantas-M
Copy link
Contributor

Description

Updated sea-orm and sea-orm-migration to 1.1.1 (latest as of opening this PR) and resolved any issues that occurred due to the version bump.

Drive-by changes

Related issues

Backward compatibility

Yes

Testing

Manual, as described in #4793

Copy link

changeset-bot bot commented Nov 30, 2024

⚠️ No Changeset found

Latest commit: ed6d556

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

let mut buf = [0u8; 32];
v.to_little_endian(&mut buf);
BigDecimal::from(BigInt::from_bytes_le(Sign::Plus, &buf as &[u8]))
let big_dec = &BigDecimal::from(BigInt::from_bytes_le(Sign::Plus, &buf as &[u8])).to_string();
Decimal::from_str(big_dec).unwrap()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can fit U256 into Decimal?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decimal and BigDecimal are both mapped to the same type on PostgreSQL, I was wondering why the change in that too but if you look at the definition of Wei (which uses BigDecimal) that has not changed from the old version.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another note on this, the BigDecimal -> Decimal was a change which was made by the entity generation of the updated sea-orm package, and has nothing do with any specific changes made to the schema.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose that the definition of column type Wei did not change since it is specified in the native Postgres type: decimal, which is equivalent of numeric ("The types decimal and numeric are equivalent"). So, Wei defines the column type, not the type which is going to be used in Rust struct which is mapped to the record in the table.

I don't believe that Rust Decimal can contain all values possible in U256. Type Decimal supports only 96-bit integers. In contrast, BigDecimal internally contains a vector of digits (see BigUint type).

I wonder if it is possible to enable feature with-bigdecimal on Postgres driver?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not from what I found though going through code and docs.

Their discord is pretty dead, but it's better than nothing so I've tried to ask there.

In general, I doubt you would be storing gas values that overflow the decimal type in rust, perhaps one thing we could do is swap it to bigint 🤔

Comment on lines 3 to 4
pub use super::{
block::Entity as Block, cursor::Entity as Cursor,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose we can commit prelude which was generated by generate_entities.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what you mean by commit could you please specify?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you run generate_entities, it will generate entities files. They will be the same as currently committed in this PR with the exception of prelude.rs file. I think that prelude.rs can be also added to this PR. Current one and the generate one are equivalent.

Copy link
Contributor Author

@Mantas-M Mantas-M Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhh, I get it now, the new prelude is added in this PR. It is just that the generated one has a line by line format, not a block format, I changed it back to the block format so we can use the allow_unused_imports on the whole block as before. You can see the update I did here ed6d556

But you are right, the generated one is the same as the old one so everything is fine in that regard. The edit is just so we avoid having the warnings from the generated prelude.rs

Hope that makes sense

Comment on lines +7 to 8
#[tokio::main(flavor = "current_thread")]
async fn main() -> Result<(), DbErr> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it produces the same database schema as before (i.e. old version vs new version)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears so, I did the test which you mentioned in the issue of the description, which is to generate a schema on the old version, scrape a bit, then switch to the new version and continue scraping, everything worked fine

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is good that it works. It is not the test for this comment though (but a good test for that comment)

The test is mostly manual for this:

  1. Start Postgres on one port.
  2. Init schema there (init_db.rs) from main.
  3. Start Postgres on a different port.
  4. Init schema from this branch.
  5. Compare the tables and field types.

Copy link
Contributor Author

@Mantas-M Mantas-M Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, okay, I have done that.

Old schema:
postgres - public old

New schema:
postgres - public new

And the BigDecimal -> Decimal change did not alter anything is the postges schema as we can see, the type is still numeric 78 which is the same as is set in the l20230309_types file
image
image

@ameten
Copy link
Contributor

ameten commented Dec 4, 2024

And it will be great to check if newly generated entities can work with old schema without issue. I can see that we have replaced BigDecimal with Decimal in some cases.

@Mantas-M
Copy link
Contributor Author

Mantas-M commented Dec 5, 2024

And it will be great to check if newly generated entities can work with old schema without issue. I can see that we have replaced BigDecimal with Decimal in some cases.

I checked it during the manual tests, scraping with the old/new versions, it was working fine ✅

@Mantas-M
Copy link
Contributor Author

Mantas-M commented Dec 5, 2024

Implementation details of Decimal vs BigDecimal
https://github.com/SeaQL/sea-orm/blob/master/src/driver/sqlx_postgres.rs#L516

@Mantas-M Mantas-M requested a review from ameten December 6, 2024 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

chore: Upgrade SeaORM to the latest version
2 participants