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

program sdk: move AddressLookupTableAccount to message::v0 #154

Closed
wants to merge 1 commit into from

Conversation

buffalojoec
Copy link

Problem

Similar to #141, the struct for AddressLookupTableAccount exported from the Address Lookup Table program crate is also only used in one place: V0 Message.

Summary of Changes

Move the struct directly into sdk::program::message::v0 as a public struct.

The struct's visibility makes it available for use in sdk::program::message as well as downstream for developers who may need the type when working with messages.

Again similar to #141, this allows the program SDK to remain decoupled from the new core BPF Address Lookup Table program crate, in development here.

@buffalojoec buffalojoec requested a review from jstarry March 8, 2024 16:07
@buffalojoec
Copy link
Author

I don't think we need to add a deprecated path to sdk/program/src/lib.rs for this struct since we're going to 2.0, but I can add that if need be.

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 81.8%. Comparing base (e027a8b) to head (511fe42).
Report is 3 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #154   +/-   ##
=======================================
  Coverage    81.8%    81.8%           
=======================================
  Files         838      838           
  Lines      226389   226389           
=======================================
+ Hits       185342   185361   +19     
+ Misses      41047    41028   -19     

@t-nelson
Copy link

t-nelson commented Mar 8, 2024

i'm not sure the motivation for the change is accurate. ALT accounts were introduced in v0 transactions. legacy tx had no way to specify them. that they're "only used in v0 trasactions" is just a function of there being no newer transaction version. ALT should remain independent of any transaction version, imo

@buffalojoec
Copy link
Author

i'm not sure the motivation for the change is accurate. ALT accounts were introduced in v0 transactions. legacy tx had no way to specify them. that they're "only used in v0 trasactions" is just a function of there being no newer transaction version. ALT should remain independent of any transaction version, imo

Fair point, but the type right below it in v0 is MessageAddressTableLookup and there's some code in v0 specific to mapping AddressLookupTableAccount elements to MessageAddressTableLookup.

Perhaps it's better to do a little refactoring and introduce a module for lookup-related items?

@t-nelson
Copy link

t-nelson commented Mar 9, 2024

right. i think that's a different problem. Message is heavily overloaded there. there are at least three specific use cases being forced onto one type. very solana.

  1. tx specification - this should just be w/e the minimum is to get a tx compiled, serialized and on the wire.
  2. runtime dynamic features - ALT loaded accounts, write lock demotions (see #84), etc. sdk need not be exposed to this stuff
  3. rpc metadata - this is pure informational and can live in an rpc client crate

@jstarry
Copy link

jstarry commented Apr 6, 2024

I don't really understand this change.. why is it important to move a struct around within the same crate?

Similar to #141, the struct for AddressLookupTableAccount exported from the Address Lookup Table program crate is also only used in one place: V0 Message.

This struct is in the solana-program crate not the "Address Lookup Table program crate"

@buffalojoec
Copy link
Author

buffalojoec commented Apr 6, 2024

I don't really understand this change.. why is it important to move a struct around within the same crate?

Similar to #141, the struct for AddressLookupTableAccount exported from the Address Lookup Table program crate is also only used in one place: V0 Message.

This struct is in the solana-program crate not the "Address Lookup Table program crate"

The idea was to then deprecate the address_lookup_table module in favor of the BPF program-client crate.

The program SDK doesn't need anything from the program, but downstream users could import the new ALT client if they needed to use the program's API.

This struct here is just used within message creation, and not needed for the program's API.

@buffalojoec buffalojoec closed this Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants