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

Update to 1.17.22 + share lib code #18

Merged
merged 4 commits into from
Feb 21, 2024
Merged

Update to 1.17.22 + share lib code #18

merged 4 commits into from
Feb 21, 2024

Conversation

ochaloup
Copy link
Contributor

This update the mono repo solana dependency to 1.17.22 (current version on mainnet).
And moving the TreeNode to be shared under merkle-tree module. With that changing the solana-sdk to solana-program as it's dependency needed for contracts.

I would be happy to hear any feedback how to better structure the project if this module splitting solution does not fit.
Plus, I'm currently not able to oversee if the change of the solana version may do some issues elsewhere.

@ochaloup ochaloup requested a review from janlegner February 15, 2024 09:02
@ochaloup ochaloup force-pushed the update-to-1.17.22 branch 3 times, most recently from 3ee434f to 75f57be Compare February 15, 2024 09:14
@@ -0,0 +1,24 @@
use {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like the change overall, I am just curious why is this wile called insurance_engine.rs? Just a copy-paste relict?

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 was not sure about name here. First I was thinking to place it under lib.rs directly but I my thoughts then went in direction that lib.rs to be a generic merkle tree stuff and then there could be added some modules that work with some specific implementation. Here the tree node comes from insurance engine module so I took that name. It was intentional but probably not good.

Would you have idea how to do better? Removing the file? Change to a better name like validator_bonds maybe?

Copy link
Collaborator

@janlegner janlegner Feb 15, 2024

Choose a reason for hiding this comment

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

Now I see. Yes, probably validator_bonds_claim, psr_claim or similar would make sense.

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 used the name psr_claim.rs
(PR code forced to check with main branch where update is related to contract deploy)

@janlegner
Copy link
Collaborator

Please let's merge this only after #17 has been merged, as that PR has more prio and it is possible we will have to spend some time resolving conflicts or making this build.

@janlegner
Copy link
Collaborator

There you go @ochaloup 😁

@ochaloup
Copy link
Contributor Author

ochaloup commented Feb 15, 2024

@butonium do you think I can (or let me know when it's possible) to merge this PR. E.g., regarding the solana version change.

NOTE: Checking of this PR is not a priority now and it can wait for next week.

I re-based the code against the #17 and run cargo fmt (please see manually fixed fmt errors in code from below[1] and as of the last commit update).

[1]

warning: field `message` is never read
  --> api/src/handlers/bonds.rs:19:5
   |
18 | struct CustomError {
   |        ----------- field in this struct
19 |     message: String,
   |     ^^^^^^^
   |
   = note: `CustomError` has a derived impl for the trait `Debug`, but this is intentionally ignored during dead code analysis
   = note: `#[warn(dead_code)]` on by default

warning: useless conversion to the same type: `rust_decimal::Decimal`
  --> api/src/repositories/bond.rs:29:20
   |
29 |             cpmpe: row.get::<_, Decimal>("cpmpe").try_into()?,
   |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: consider removing `.try_into()`
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_conversion
   = note: `#[warn(clippy::useless_conversion)]` on by default

warning: `api` (lib) generated 2 warnings
warning: `api` (lib test) generated 2 warnings (2 duplicates)

@butonium
Copy link
Contributor

@butonium do you think I can (or let me know when it's possible) to merge this PR. E.g., regarding the solana version change.

NOTE: Checking of this PR is not a priority now and it can wait for next week.

I re-based the code against the #17 and run cargo fmt (please see manually fixed fmt errors in code from below[1] and as of the last commit update).

[1]

warning: field `message` is never read
  --> api/src/handlers/bonds.rs:19:5
   |
18 | struct CustomError {
   |        ----------- field in this struct
19 |     message: String,
   |     ^^^^^^^
   |
   = note: `CustomError` has a derived impl for the trait `Debug`, but this is intentionally ignored during dead code analysis
   = note: `#[warn(dead_code)]` on by default

warning: useless conversion to the same type: `rust_decimal::Decimal`
  --> api/src/repositories/bond.rs:29:20
   |
29 |             cpmpe: row.get::<_, Decimal>("cpmpe").try_into()?,
   |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: consider removing `.try_into()`
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_conversion
   = note: `#[warn(clippy::useless_conversion)]` on by default

warning: `api` (lib) generated 2 warnings
warning: `api` (lib test) generated 2 warnings (2 duplicates)

I think my rust-analyzer got fried a bit cause I missed those. Regarding this PR I am not sure how it will affect deserialization, other than that, lgtm

@ochaloup
Copy link
Contributor Author

Resolution after check with @butonium - waiting with merge for the next week when things calm down.

@ochaloup ochaloup merged commit 844e335 into main Feb 21, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants