Add interface crate #47

kevinheavey · 2025-04-03T10:01:00Z

Moving from anza-xyz/solana-sdk#123

joncinque

Looks good to me overall, just a few little things

interface/src/config_instruction.rs

interface/Cargo.toml

interface/src/lib.rs

joncinque

Looks good to me! But do get approval from @buffalojoec as well

buffalojoec

I just did some work on removing the Config builtin from Agave before you opened this, so there are a few updates we can make here.

For starters, the builtin crates are all non-semver, and we're also just outright dropping them, so no need to preserve any backwards compatibility in this new interface crate.

create_config_account
serialized_size
Anything else that seems silly to include in the interface API

I also outfitted the Rust client in this repository to support the remaining builtin API we needed in Agave, so that should already have all of the methods we need in the validator.

That being said, I'm sure the Rust client in here is bringing a lot of dependencies we don't need. Can we just refactor the client, or is the interface crate absolutely necessary?

interface/src/state.rs

buffalojoec · 2025-04-04T06:11:10Z

interface/src/instruction.rs

https://github.com/solana-program/config/blob/main/clients/rust/src/instructions_bincode.rs

buffalojoec

Let's either refactor the existing client or strip this new crate down to bare bones, as per my initial comment.

kevinheavey · 2025-04-04T11:07:06Z

I removed create_config_account and the serialized_size method. Looks like everything else can still be used in Agave.

I don't know how to remove solana-program from the existing client since it's autogenerated

buffalojoec · 2025-04-07T04:59:23Z

Welp, evicting solana-program from Codama is going to be a bit of a job, but it's probably the right thing to do, since all of these program clients use it. I kicked off the discussion and the process over here: codama-idl/codama#535.

I'm not convinced jamming an interface crate into this repository, which contains the same API as the client, just with better dependency management, is the right way to go. If it was, we might as well just stop using the Codama clients altogether. The long-tail, right thing to do is probably to see my PR into Codama all the way through and get those clients refactored properly, so they aren't introducing kitchen sink SDKs into dependent libraries, including those in Agave.

If that's not something you can wait for, and you need crates in Agave un-bloated sooner, we can continue discussion about some kind of shorter-term workaround (like this PR). Which crate(s) are you primarily seeking to slim down?

kevinheavey · 2025-04-07T10:10:19Z

we might as well just stop using the Codama clients altogether

The only auto-generated part of solana_config_program_client that gets used in Agave is the program ID - the rest is hand-written Rust. So yes that would be a good idea here at least.

Which crate(s) are you primarily seeking to slim down?

The main one is solana-account-decoder. yellowstone-grpc depends on this via solana-transaction-status (and it is using types that are not present in solana-transaction-status-client-types).

joncinque · 2025-04-07T12:27:22Z

Typically we've been using the interface crates to define whatever's necessary to create the IDL, from which we can auto-generate clients, ie https://github.com/solana-program/system/tree/main/interface

We need to have the base types somewhere from which the IDL is generated, otherwise we have a circular relationship. Meaning, if the IDL is generated from the Rust client, and then Rust client is generated from the IDL, we're going to run into weird situations.

Why not put those base things in here?

kevinheavey · 2025-04-16T12:00:47Z

Why not put those base things in here?

@joncinque Is that a suggestion to change something in this PR?

buffalojoec · 2025-04-21T12:40:13Z

The only auto-generated part of solana_config_program_client that gets used in Agave is the program ID - the rest is hand-written Rust. So yes that would be a good idea here at least.

Yes, that's only because I was too lazy to refactor all of the hand-written stuff in Agave to use the generated stuff. 😅 Maybe that's a good thing for what this PR aims to do though.

I was going to write up a few suggestions, but I should have probably done some of this when I first added the hooked types to the client, instead of just dumping them all in the dependency-bloated Codama client. So instead I just added a commit to deprecate the instructions_bincode from the client and re-export from the interface.

My next commit would be to squash the reimplementations of ShortU16, ShortVec and ConfigKeys from the client into the interface, with their borsh implementations behind a new borsh feature. However, that feels annoying, since there is already full serde (and serde_with(short_vec)) support from the SDK API. The only reason for the reimplementations is the client's annoying dependency on borsh. So I think I'm okay with not doing that actually.

Lmk what you think of my commit, otherwise g2g for me @kevinheavey

buffalojoec · 2025-04-21T12:42:37Z

interface/src/state.rs

+    fn max_space() -> u64;
+}
+
+// TODO move ConfigState into `solana_program` to implement trait locally


Ehh, we should probably just get rid of the trait. It's pretty silly.

I'm definitely down to remove it, but where should we move it to in the meantime? It's still used a bit in agave

Probably just somewhere in Agave. I'll take a look and circle back here!

buffalojoec · 2025-04-21T12:43:21Z

program/src/state.rs

-    #[serde(with = "short_vec")]
-    pub keys: Vec<(Pubkey, bool)>,
-}
+pub use solana_config_interface::state::ConfigKeys;


The program crate has not been published for 3.0.0, so we can actually drop this altogether!

Wow, that means the program won't actually depend on the interface at all, right? Man, the config program is weird

joncinque

Looks good to me, mainly just the question about the plan for the trait if you want to remove it

joncinque · 2025-04-22T21:03:36Z

program/src/state.rs

-    #[serde(with = "short_vec")]
-    pub keys: Vec<(Pubkey, bool)>,
-}
+pub use solana_config_interface::state::ConfigKeys;


Wow, that means the program won't actually depend on the interface at all, right? Man, the config program is weird

joncinque · 2025-04-22T21:05:43Z

interface/src/state.rs

+    fn max_space() -> u64;
+}
+
+// TODO move ConfigState into `solana_program` to implement trait locally


I'm definitely down to remove it, but where should we move it to in the meantime? It's still used a bit in agave

joncinque · 2025-04-22T21:06:51Z

interface/src/instruction.rs

@@ -29,14 +21,14 @@ pub fn create_account<T: ConfigState>(
    lamports: u64,
    keys: Vec<(Pubkey, bool)>,
 ) -> Vec<Instruction> {
-    let space = T::max_space().saturating_add(serialized_size(&ConfigKeys { keys }).unwrap());
+    let space = T::max_space() + bincode::serialized_size(&ConfigKeys { keys }).unwrap();


Was this change intended?

kevinheavey added 2 commits April 3, 2025 10:48

add interface crate

8ed4646

use solana-config-interface in solana-config-program

c2c926b

kevinheavey requested a review from joncinque April 3, 2025 10:17

joncinque requested a review from buffalojoec April 3, 2025 11:39

joncinque reviewed Apr 3, 2025

View reviewed changes

kevinheavey added 3 commits April 3, 2025 14:11

rename config_instruction to instruction

6cab4c2

explicit feature activation

f5f394e

move to state.rs

784f287

joncinque previously approved these changes Apr 3, 2025

View reviewed changes

buffalojoec reviewed Apr 4, 2025

View reviewed changes

interface/src/state.rs Outdated Show resolved Hide resolved

interface/src/state.rs Outdated Show resolved Hide resolved

buffalojoec reviewed Apr 4, 2025

View reviewed changes

buffalojoec requested changes Apr 4, 2025

View reviewed changes

remove code no longer required externally

df33b22

client: deprecate instructions_bincode

2181cb4

buffalojoec reviewed Apr 21, 2025

View reviewed changes

kevinheavey dismissed joncinque’s stale review via 2181cb4 April 22, 2025 20:33

joncinque reviewed Apr 22, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add interface crate #47

Add interface crate #47

kevinheavey commented Apr 3, 2025

joncinque left a comment

joncinque left a comment

buffalojoec left a comment

buffalojoec Apr 4, 2025

buffalojoec left a comment

kevinheavey commented Apr 4, 2025

buffalojoec commented Apr 7, 2025

kevinheavey commented Apr 7, 2025

joncinque commented Apr 7, 2025

kevinheavey commented Apr 16, 2025

buffalojoec commented Apr 21, 2025

buffalojoec Apr 21, 2025

joncinque Apr 22, 2025

buffalojoec Apr 23, 2025

buffalojoec Apr 21, 2025

joncinque Apr 22, 2025

joncinque left a comment

joncinque Apr 22, 2025

joncinque Apr 22, 2025

joncinque Apr 22, 2025

Add interface crate #47

Are you sure you want to change the base?

Add interface crate #47

Conversation

kevinheavey commented Apr 3, 2025

joncinque left a comment

Choose a reason for hiding this comment

joncinque left a comment

Choose a reason for hiding this comment

buffalojoec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buffalojoec left a comment

Choose a reason for hiding this comment

kevinheavey commented Apr 4, 2025

buffalojoec commented Apr 7, 2025

kevinheavey commented Apr 7, 2025

joncinque commented Apr 7, 2025

kevinheavey commented Apr 16, 2025

buffalojoec commented Apr 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joncinque left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment