-
Notifications
You must be signed in to change notification settings - Fork 4
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
Conversation
3ee434f
to
75f57be
Compare
merkle-tree/src/insurance_engine.rs
Outdated
@@ -0,0 +1,24 @@ | |||
use { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
75f57be
to
23830ce
Compare
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. |
There you go @ochaloup 😁 |
23830ce
to
8618ba7
Compare
@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 [1]
|
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 |
Resolution after check with @butonium - waiting with merge for the next week when things calm down. |
This update the mono repo solana dependency to 1.17.22 (current version on mainnet).
And moving the
TreeNode
to be shared undermerkle-tree
module. With that changing thesolana-sdk
tosolana-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.