-
Notifications
You must be signed in to change notification settings - Fork 381
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
sdk: evict rent collector #4160
sdk: evict rent collector #4160
Conversation
3d3dbec
to
951c8c7
Compare
5574dec
to
c137850
Compare
c137850
to
76be235
Compare
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.
Looks good! Just some small things
extern crate log as logger; | ||
#[cfg_attr(not(target_os = "solana"), macro_use)] | ||
extern crate serde_derive; | ||
|
||
#[cfg_attr(feature = "frozen-abi", macro_use)] | ||
#[cfg(feature = "frozen-abi")] | ||
extern crate solana_frozen_abi_macro; |
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.
We can probably remove all of these, but it seems unrelated to this PR -- can you split it out into another PR?
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.
It was a clippy error, but sure.
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.
It will not pass CI here if it's not part of this PR, though.
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.
Ooh, this is actually beautiful! This is the last module still using frozen-abi and serde-derive, we can remove frozen-abi and move serde-derive to dev-deps. I can put that change in once yours goes in.
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.
Sounds good!
@@ -22,7 +22,7 @@ pub mod tests { | |||
|
|||
#[test] | |||
fn test_limited_deserialize() { | |||
#[derive(Deserialize, Serialize)] | |||
#[derive(serde_derive::Deserialize, serde_derive::Serialize)] |
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.
Same with this, unless I'm missing something, it should get split out into another PR.
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.
Also clippy.
Co-authored-by: Jon C <[email protected]>
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.
Looks great!
extern crate log as logger; | ||
#[cfg_attr(not(target_os = "solana"), macro_use)] | ||
extern crate serde_derive; | ||
|
||
#[cfg_attr(feature = "frozen-abi", macro_use)] | ||
#[cfg(feature = "frozen-abi")] | ||
extern crate solana_frozen_abi_macro; |
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.
Ooh, this is actually beautiful! This is the last module still using frozen-abi and serde-derive, we can remove frozen-abi and move serde-derive to dev-deps. I can put that change in once yours goes in.
Completes the work for #3932 and finally evicts
RentCollector
from the SDK.