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.

Sign up for GitHub

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

Jump to bottom

Permit paying oneself #10337

Merged
merged 3 commits into from
May 31, 2020
Merged

Permit paying oneself #10337

merged 3 commits into from
May 31, 2020

Conversation

garious
Copy link
Contributor

@garious garious commented May 30, 2020 •
edited
Loading

Problem

Can't transfer SOL to oneself. A while back, it wasn't possible to hold two references to the same account, and we went as far as ensuring that returned a nice error code (3c6af52). Now that's no problem, but the System instruction program still holds a write lock on the to account for no obvious reason, preventing the program from capturing the from lock.

Summary of Changes

Permit pay-to-self transactions.

Towards #10336

@garious garious requested a review from jackcmay May 30, 2020 04:25
jackcmay
jackcmay previously approved these changes May 30, 2020
View reviewed changes
Copy link
Contributor

@jackcmay jackcmay left a comment

Choose a reason for hiding this comment

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

@garious You're right, I was keeping the semantics but didn't notice the self pay case. Passing the KeyedAccount seems cleaner and clearer anyway. Was an account passed to make the tests simpler? Maybe because only an account was originally necessary?

@codecov
Copy link

codecov bot commented May 30, 2020 •
edited
Loading

Codecov Report

Merging #10337 into master will decrease coverage by 0.0%.
The diff coverage is 93.7%.

@@            Coverage Diff            @@
##           master   #10337     +/-   ##
=========================================
- Coverage    81.3%    81.3%   -0.1%     
=========================================
  Files         288      288             
  Lines       67000    67094     +94     
=========================================
+ Hits        54510    54584     +74     
- Misses      12490    12510     +20

@mvines mvines mentioned this pull request May 30, 2020
Closed
@mergify mergify bot dismissed jackcmay’s stale review May 30, 2020 14:05

Pull request has been modified.

@garious garious added the automerge Merge this Pull Request automatically once CI passes label May 30, 2020
@solana-grimes solana-grimes removed the automerge Merge this Pull Request automatically once CI passes label May 30, 2020
@solana-grimes
Copy link
Contributor

💔 Unable to automerge due to CI failure

@garious
Allow paying to oneself
901f7a4
@garious
cargo fmt
7c4b6de
@garious
Permit pay-to-self in CLI
2bc3d2d 
No test here because we're just removing an [untested] special case.

Fixes solana-labs#10339
@garious garious added the automerge Merge this Pull Request automatically once CI passes label May 30, 2020
@solana-grimes
Copy link
Contributor

💔 Unable to automerge due to CI failure

@solana-grimes solana-grimes removed the automerge Merge this Pull Request automatically once CI passes label May 30, 2020
@garious garious added the automerge Merge this Pull Request automatically once CI passes label May 30, 2020
@solana-grimes
Copy link
Contributor

💔 Unable to automerge due to CI failure

@solana-grimes solana-grimes removed the automerge Merge this Pull Request automatically once CI passes label May 30, 2020
CriesofCarrots
CriesofCarrots approved these changes May 30, 2020
View reviewed changes
Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

Sweet
Fixes #10339

cli/src/cli.rs
Comment on lines -1533 to -1537
check_unique_pubkeys(
(&from.pubkey(), "cli keypair".to_string()),
(to, "to".to_string()),
)?;

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove this check from process_pay as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

About time we rip out all things Budget. All that code can have a new home in the SPL. We can add a new solana-budget CLI as well.

@garious garious added the automerge Merge this Pull Request automatically once CI passes label May 30, 2020
@solana-grimes
Copy link
Contributor

💔 Unable to automerge due to CI failure

@solana-grimes solana-grimes removed the automerge Merge this Pull Request automatically once CI passes label May 31, 2020
@garious garious merged commit 5d248fe into solana-labs:master May 31, 2020
mergify bot pushed a commit that referenced this pull request May 31, 2020
@garious
Permit paying oneself (#10337)
8ac23ac 
* Allow paying to oneself

* cargo fmt

* Permit pay-to-self in CLI

No test here because we're just removing an [untested] special case.

Fixes #10339

(cherry picked from commit 5d248fe)

# Conflicts:
#	runtime/src/system_instruction_processor.rs
@mergify mergify bot mentioned this pull request May 31, 2020
Merged
mergify bot pushed a commit that referenced this pull request May 31, 2020
@garious
Permit paying oneself (#10337)
aa6cd96 
* Allow paying to oneself

* cargo fmt

* Permit pay-to-self in CLI

No test here because we're just removing an [untested] special case.

Fixes #10339

(cherry picked from commit 5d248fe)
@mergify mergify bot mentioned this pull request May 31, 2020
Merged
solana-grimes pushed a commit that referenced this pull request May 31, 2020
@mergify
Permit paying oneself (#10337) (#10342)
94ba700 
automerge
garious added a commit that referenced this pull request May 31, 2020
@garious
Permit paying oneself (#10337)
82ce035 
* Allow paying to oneself

* cargo fmt

* Permit pay-to-self in CLI

No test here because we're just removing an [untested] special case.

Fixes #10339

(cherry picked from commit 5d248fe)
solana-grimes pushed a commit that referenced this pull request May 31, 2020
@mergify
Permit paying oneself (#10337) (#10341)
640bb9c 
automerge
This was referenced Jun 2, 2020
Closed
Merged
@ryoqun ryoqun mentioned this pull request Jun 9, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CriesofCarrots CriesofCarrots CriesofCarrots approved these changes

@jackcmay jackcmay jackcmay left review comments

@aeyakovenko aeyakovenko Awaiting requested review from aeyakovenko

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@garious @solana-grimes @CriesofCarrots @jackcmay