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

Add test for program_ids passed in metas #8618

Merged

Conversation

jackcmay
Copy link
Contributor

@jackcmay jackcmay commented Mar 4, 2020

Problem

Account loading filters out any accounts associated with program_ids. If the instruction's account_keys references a program_id (aka, attempts to pass a program_id to a program) the message processor will panic, boo.

Summary of Changes

The first step is to add a test that validates this behavior. This PR includes that test.

As part of the cross-program invocation work, we need the ability to pass program accounts to programs. When those changes come in this test will be expected to pass.

Fixes #

@codecov
Copy link

codecov bot commented Mar 4, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@4f05f08). Click here to learn what that means.
The diff coverage is 86.2%.

@@           Coverage Diff            @@
##             master   #8618   +/-   ##
========================================
  Coverage          ?     80%           
========================================
  Files             ?     256           
  Lines             ?   55759           
  Branches          ?       0           
========================================
  Hits              ?   44628           
  Misses            ?   11131           
  Partials          ?       0

@jackcmay
Copy link
Contributor Author

jackcmay commented Mar 4, 2020

@mvines @garious This change is a lead-up to allowing program accounts to be passed to programs by removing/modifying this line:

.filter(|(_, key)| !message.program_ids().contains(key))

Do either of you know the history behind this filtering and if there is a compelling reason to be doing it?

@garious
Copy link
Contributor

garious commented Mar 4, 2020

It looks to be saying that executable accounts are rent exempt. Maybe @ParthDesai or @rob-solana would care to comment?

@jackcmay
Copy link
Contributor Author

jackcmay commented Mar 4, 2020

@garious Yeah, it seems to be two-fold (not loading and rent exempt). I have a follow-up PR that I think addresses both, partially to help cross-program-invocations but also to get rid of this:

// TODO: panics on an index out of bounds if an executable

@jackcmay
Copy link
Contributor Author

jackcmay commented Mar 4, 2020

This PR only adds a test to verify the current behavior so will commit without approval, I'll post the follow-up PR where instructions are able to pass program_ids. Looking forward to feedback on that one from @ParthDesai and @rob-solana

@jackcmay jackcmay merged commit 408d5da into solana-labs:master Mar 4, 2020
@jackcmay jackcmay deleted the add-test-for-program-id-in-metas branch March 4, 2020 19:13
@jackcmay
Copy link
Contributor Author

jackcmay commented Mar 4, 2020

Follow-up PR:
#8639

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