Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Don't require program_id to passed down cpi call chain#34191

Closed
seanyoung wants to merge 2 commits intosolana-labs:masterfrom
seanyoung:dont-require-program-id-instruction
Closed

Don't require program_id to passed down cpi call chain#34191
seanyoung wants to merge 2 commits intosolana-labs:masterfrom
seanyoung:dont-require-program-id-instruction

Conversation

@seanyoung
Copy link
Copy Markdown
Contributor

Problem

When using cpi, the callee program id must be passed down the call chain. Remove this restriction. See #34057

Summary of Changes

Find the program_id in the transaction context rather than in the instruction context.

Fixes #

@seanyoung seanyoung added the feature-gate Pull Request adds or modifies a runtime feature gate label Nov 21, 2023
@seanyoung seanyoung requested a review from Lichtso November 21, 2023 10:03
@seanyoung seanyoung force-pushed the dont-require-program-id-instruction branch 3 times, most recently from 83a622b to 9c6ee2a Compare November 21, 2023 10:36
Copy link
Copy Markdown
Contributor

@Lichtso Lichtso left a comment

Choose a reason for hiding this comment

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

Some nits, otherwise looking good.

Comment thread sdk/src/feature_set.rs Outdated
(validate_fee_collector_account::id(), "validate fee collector account #33888"),
(disable_rent_fees_collection::id(), "Disable rent fees collection #33945"),
(enable_zk_transfer_with_fee::id(), "enable Zk Token proof program transfer with fee"),
(program_id_not_required_in_instruction::id(), "Don't require a program id to be passed down the cpi chain"),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

program_id_not_required_in_instruction suggests that you don't require a program key in instructions at all anymore. Maybe a better name would be "lift program key in parent instruction restriction" or something like that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've struggled to come up with a good name. I've called it no_need_for_program_key_in_parent_instruction. I'm open to ideas here, it's not great

Comment thread programs/sbf/tests/programs.rs Outdated
// Here we are not passing caller2_pubkey down, but cpi should still succeed
let account_metas = vec![
AccountMeta::new_readonly(caller_pubkey, false),
AccountMeta::new_readonly(caller2_pubkey, false),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

But this is passing caller2_pubkey down right here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, that was not great. I've created some better tests below

@seanyoung seanyoung force-pushed the dont-require-program-id-instruction branch 6 times, most recently from c55e622 to f95730c Compare November 21, 2023 16:31
@seanyoung seanyoung force-pushed the dont-require-program-id-instruction branch from f95730c to 90a7fc4 Compare November 21, 2023 16:53
@codecov
Copy link
Copy Markdown

codecov Bot commented Nov 21, 2023

Codecov Report

Merging #34191 (90a7fc4) into master (504f2ee) will decrease coverage by 0.1%.
Report is 4 commits behind head on master.
The diff coverage is 80.8%.

Additional details and impacted files
@@            Coverage Diff            @@
##           master   #34191     +/-   ##
=========================================
- Coverage    81.9%    81.9%   -0.1%     
=========================================
  Files         819      819             
  Lines      219380   219466     +86     
=========================================
+ Hits       179773   179810     +37     
- Misses      39607    39656     +49     

@willhickey
Copy link
Copy Markdown
Contributor

This repository is no longer in use. Please re-open this pull request in the agave repo: https://github.com/anza-xyz/agave

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

feature-gate Pull Request adds or modifies a runtime feature gate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants