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

Remove create_account bandaid now that to's signature is required #7776

Merged
merged 2 commits into from
Jan 15, 2020

Conversation

rob-solana
Copy link
Contributor

@rob-solana rob-solana commented Jan 13, 2020

Problem(s)

create_account() checks to balance, but shouldn't as long as
there's no existing data and the system account is the owner (redundant with
verify_account_changes(), but no biggie)

Summary of Changes

remove the balance check for to

Fixes #

@rob-solana rob-solana changed the title Remove create account bandaid now that to requires signature Remove create_account bandaid now that to requires signature Jan 13, 2020
@codecov
Copy link

codecov bot commented Jan 13, 2020

Codecov Report

Merging #7776 into master will decrease coverage by <.1%.
The diff coverage is 100%.

@@           Coverage Diff            @@
##           master   #7776     +/-   ##
========================================
- Coverage    81.8%   81.8%   -0.1%     
========================================
  Files         239     242      +3     
  Lines       51203   51236     +33     
========================================
+ Hits        41932   41959     +27     
- Misses       9271    9277      +6

t-nelson
t-nelson previously approved these changes Jan 14, 2020
Copy link
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

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

lgtm!

@rob-solana rob-solana changed the title Remove create_account bandaid now that to requires signature Remove create_account bandaid now that to's signature is required Jan 14, 2020
runtime/src/system_instruction_processor.rs Show resolved Hide resolved
@@ -147,7 +147,7 @@ pub fn create_account(
program_id: &Pubkey,
) -> Instruction {
let account_metas = vec![
AccountMeta::new(*from_pubkey, true),
AccountMeta::new(*from_pubkey, lamports != 0), // no signature required if no transfer
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the point of this feature exactly? And if you use it, why require from_pubkey?

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 could round out system program with another set of functions/instructions that don't require a from, but I think they'd only manifest as system_instruction.rs entry points, as so many things funnel down.

My next PR makes clear the value of this feature. Stay tuned!

Copy link
Contributor

Choose a reason for hiding this comment

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

Something still smells funny here. Is it possible that CreateAccount shouldn't have from or lamports parameters at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

your new issue nicely describes what I'm going for here

Copy link
Contributor

Choose a reason for hiding this comment

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

Good to hear! So baby steps, how about we leave CreateAccount alone and instead add the proposed Allocate instruction in this PR?

@mergify mergify bot dismissed t-nelson’s stale review January 15, 2020 19:24

Pull request has been modified.

Copy link
Contributor

@garious garious left a comment

Choose a reason for hiding this comment

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

Well played, Sir

This was referenced May 14, 2024
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