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

Make solana-address-lookup-table-program crate bpf compatible #23700

Merged
merged 1 commit into from
Mar 17, 2022

Conversation

jstarry
Copy link
Member

@jstarry jstarry commented Mar 16, 2022

Problem

The solana-address-lookup-table-program crate cannot be used as a dependency when building bpf programs

Summary of Changes

  • Use cfg attributes to disable BPF incompatible dependencies and instruction processing
  • Moved AddressLookupError to the solana-address-lookup-table-program to make state types usable without a solana-sdk dependency.
  • Added solana-address-lookup-table-program as a dependency for the dep-crate test program to ensure that compatibility is preserved in the future

Fixes #23410

@jstarry jstarry requested a review from joncinque March 16, 2022 08:13
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

looks good + one nit. This might actually be a better way forward than the current design of "just stick everything in sdk / program".

Instead of using target_arch everywhere, as another option, we could use the no-entrypoint feature as a convention even for builtin programs, so that pulling in program-runtime and sdk is dependent on the feature instead of the bpf arch. What do you think?

Approving either way since the changes are good.

Comment on lines +24 to +34
#[cfg(not(target_arch = "bpf"))]
impl From<AddressLookupError> for TransactionError {
fn from(err: AddressLookupError) -> Self {
match err {
AddressLookupError::LookupTableAccountNotFound => Self::AddressLookupTableNotFound,
AddressLookupError::InvalidAccountOwner => Self::InvalidAddressLookupTableOwner,
AddressLookupError::InvalidAccountData => Self::InvalidAddressLookupTableData,
AddressLookupError::InvalidLookupIndex => Self::InvalidAddressLookupTableIndex,
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: what do you think about keeping just this part in solana-sdk? Then you can avoid at least one target_arch cfg

Copy link
Member Author

Choose a reason for hiding this comment

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

Then solana-sdk would need to depend on solana-address-lookup-table-program and there would be a circular dependency

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah right, of course

Comment on lines +15 to +16
# list of crates which must be buildable for bpf programs
solana-address-lookup-table-program = { path = "../../../../programs/address-lookup-table", version = "=1.10.3" }
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a great idea!

@jstarry
Copy link
Member Author

jstarry commented Mar 16, 2022

Instead of using target_arch everywhere, as another option, we could use the no-entrypoint feature as a convention even for builtin programs, so that pulling in program-runtime and sdk is dependent on the feature instead of the bpf arch. What do you think?

I did consider that and then considered renaming to no-processor but then finally decided on target arch approach because the abi crates already did that and it felt cleaner than forcing some consumers to know about what feature to enable

@joncinque
Copy link
Contributor

I did consider that and then considered renaming to no-processor but then finally decided on target arch approach because the abi crates already did that and it felt cleaner than forcing some consumers to know about what feature to enable

Makes sense, thanks for the explanation

@codecov
Copy link

codecov bot commented Mar 17, 2022

Codecov Report

Merging #23700 (c6f0d07) into master (2da4e3e) will decrease coverage by 0.0%.
The diff coverage is 51.5%.

@@            Coverage Diff            @@
##           master   #23700     +/-   ##
=========================================
- Coverage    81.8%    81.7%   -0.1%     
=========================================
  Files         581      582      +1     
  Lines      158312   158266     -46     
=========================================
- Hits       129518   129364    -154     
- Misses      28794    28902    +108     

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.

Make solana-address-lookup-table-program usable in programs
2 participants