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

followup safety checks for #23295 #23321

Merged
merged 1 commit into from
Feb 25, 2022

Conversation

t-nelson
Copy link
Contributor

Problem

#23295 isn't as safe as it could be

Summary of Changes

add marker trait with trait bounds on our explicit traits to ensure inner type conforms

h/t @jstarry

Comment on lines 255 to 261
BuiltinFeatureTransition::new_remove_or_retain(
"secp256k1_program",
solana_sdk::secp256k1_program::id(),
dummy_process_instruction,
feature_set::secp256k1_program_enabled::id(),
feature_set::prevent_calling_precompiles_as_programs::id(),
),
Copy link
Member

Choose a reason for hiding this comment

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

Please construct the enum variants directly here without the constructor methods. The constructor methods make it difficult to know what each feature arg is used for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm I was trying to avoid exposing implementation details. Figured it's fine because all of the behavior is selfcontained

Copy link
Member

Choose a reason for hiding this comment

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

I think my original version was fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sigh. I can hold my nose for a Rust release cycle

Copy link
Member

Choose a reason for hiding this comment

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

Ha, sorry man. I just don't trust that things won't get refactored and broken again before it's all removed.

@t-nelson t-nelson force-pushed the autotrait-resovler-break branch from b3b946f to 020dd1c Compare February 24, 2022 07:06
@t-nelson t-nelson force-pushed the autotrait-resovler-break branch from 020dd1c to a219b70 Compare February 24, 2022 20:30
@codecov
Copy link

codecov bot commented Feb 25, 2022

Codecov Report

Merging #23321 (a219b70) into master (2996f1f) will increase coverage by 0.0%.
The diff coverage is 90.3%.

@@           Coverage Diff           @@
##           master   #23321   +/-   ##
=======================================
  Coverage    81.3%    81.3%           
=======================================
  Files         571      572    +1     
  Lines      155738   155786   +48     
=======================================
+ Hits       126682   126757   +75     
+ Misses      29056    29029   -27     

@t-nelson t-nelson merged commit 5e0086c into solana-labs:master Feb 25, 2022
@t-nelson t-nelson deleted the autotrait-resovler-break branch February 25, 2022 00:51
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.

2 participants