Rename Pubkey to Address

[febo]

Problem

While a Solana address is represented by a [u8; 32] array, addresses are currently represented by a "wrapper" Pubkey type over the [u8; 32] array. Pinocchio on the other hand, defines its own Pubkey type as an alias to [u8; 32], which is considered a different type. The main motivation for that is to avoid bringing unnecessary dependencies to programs.

Additionally, there is confusion to what a pubkey represent (#204) since "some pubkeys aren't pubkeys."

Solution

This PR addresses both concerns by moving the current Pubkey type and renaming it to Address into a new crate solana-address. All dependencies in the new crate are activated by features, therefore a program has a fine-grained control of which dependencies are active.

To mitigate the impact of this refactoring, solana-pubkey re-exports everything from solana-address.

Note

The PR updates the ABI digest value of types using the Pubkey, since the type is now an alias to Address. There were also changes to the wasm bindings.

cc: @kevinheavey

[febo]

The "error" feature is needed for syscalls since the helpers return an AddressError (used to be PubkeyError) instead of a ProgramError. I think the differentiation is there since these helpers can be used offchain. We could make the on-chain "version" of those helpers return a ProgramError so programs don't need the "error" feature in a future PR.

[joncinque]

I'll be taking over the PR from here since Febo's out -- @buffalojoec can you take a look when you have a moment?

[buffalojoec]

The changes look clean to me, just few things and a question about the compatibility!

To mitigate the impact of this refactoring, solana-pubkey re-exports everything from solana-address.

I wonder if this is even worth doing. We're essentially publishing a pass-through crate, when we can just not publish a new solana-pubkey. If SDK v3 has a bunch of breaking changes, why not just make the migration from Pubkey to Address part of that upgrade process for program devs?

[buffalojoec]

Cool! I like how every dependency is optional! However I think you're going to annoy a lot of developers if you don't include a default. I recommend at least decode, but you could also make a case for std.

We can build hyper-modular tooling to empower devs who want to optimize their programs, but that doesn't mean we need to increase the burden on the majority of developers.

[joncinque]

Yeah that's a good point. My thinking was that most people will use solana-pubkey for a while, so that'll act like the "default" feature as people port over.

Since it's a breaking change to remove features from the default, I'd prefer to keep this lean for now. We can always add default features later as people struggle. What do you think?

[buffalojoec]

I guess I'll leave it up to you, but if you have a small set of sensible defaults, and power users aren't happy with those, what's the reason you'd ever have to remove a default feature because someone couldn't get around it with default-features = false?

[joncinque]

If SDK v3 has a bunch of breaking changes, why not just make the migration from Pubkey to Address part of that upgrade process for program devs?

That's a non-starter in my eyes. It would mean having to replace every instance of Pubkey with Address in order to upgrade to v3, which is already going to be a pain. If we publish both, then people have time to port things over to Address before we remove Pubkey with the next breaking change.

[joncinque]

I also moved new_rand back to solana_pubkey at the same time

[buffalojoec]

If SDK v3 has a bunch of breaking changes, why not just make the migration from Pubkey to Address part of that upgrade process for program devs?

That's a non-starter in my eyes. It would mean having to replace every instance of Pubkey with Address in order to upgrade to v3, which is already going to be a pain. If we publish both, then people have time to port things over to Address before we remove Pubkey with the next breaking change.

Okay, works for me. The churn is inevitable if solana-pubkey is doomed, but I won't argue against giving people more time.

[apfitzge]

left a comment on script change. overall change looks correct.

However, I'm not sure I understand the motivation here.

While a Solana address is represented by a [u8; 32] array, addresses are currently represented by a "wrapper" Pubkey type over the [u8; 32] array. Pinocchio on the other hand, defines its own Pubkey type as an alias to [u8; 32], which is considered a different type.

The new address struct has the exact same problem here, right?
The only benefit seems to be clarification in naming, since they are not always the public side of pub-prv keys, just addresses.
That benefit doesn't seem worth the trouble this will likely cause though. Is there something I'm missing in here?

[joncinque]

That benefit doesn't seem worth the trouble this will likely cause though. Is there something I'm missing in here?

The motivation is really to unite the Pubkey type used by both Pinocchio and the SDK, which will make it much easier to write framework-agnostic code.

There was repeated feedback from audits that we should use a newtype instead of a type alias, and since solana_pubkey::Pubkey is already a newtype, Pinocchio can just use that instead.

[apfitzge]

There was repeated feedback from audits that we should use a newtype instead of a type alias, and since solana_pubkey::Pubkey is already a newtype, Pinocchio can just use that instead.

It can only use the same type by adding a dependency - wasn't (at least initially) part of the point of pinocchio to have no strings attached? Maybe that was just a remnant of our mono-sdk?

A downside I see of keeping a newtype is that different versions will still be "different", i.e. v3.0 Address is NOT the same as v3.1 Address, so still requires devs to find/use the correct versions of crates that have consistent versions of Address.
Maybe with split sdk crates these versions will not need to change often though.
Are we going to actually use separate crate versions in 3.0+?

[joncinque]

A downside I see of keeping a newtype is that different versions will still be "different", i.e. v3.0 Address is NOT the same as v3.1 Address, so still requires devs to find/use the correct versions of crates that have consistent versions of Address

What do you mean by this? Although the types can be different based on how versions are declared, ie. one crate asks for solana-address = "=3.0" and another asks for solana-address = "=3.1", but if people are using default dependency declarations, solana-address = "3.0" and solana-address = "3.1" will resolve to the same package and work out fine.

Maybe with split sdk crates these versions will not need to change often though.

That's one of our big hopes. It's only because we made breaking changes in the wasm side that we actually need to publish v3 of all the crates, otherwise we could have ridden just fine with v2.

Are we going to actually use separate crate versions in 3.0+?

That's my hope!

[apfitzge]

What do you mean by this? Although the types can be different based on how versions are declared, ie. one crate asks for solana-address = "=3.0" and another asks for solana-address = "=3.1", but if people are using default dependency declarations, solana-address = "3.0" and solana-address = "3.1" will resolve to the same package and work out fine.

I don't recall what (possibly community) libraries I was using, but I've hit something like this with a useless error message:
expected struct solana_sdk::pubkey::Pubkey, got struct solana_sdk::pubkey::Pubkey

(might have been Hash, but it was one of the really fundamental types). came down to different minor versions being used in the crates. Just want to avoid that since it's a huge dev pain to hit these things. In hindsight it was probably because sdk was a mono and they needed to pin to a specific version because we broke something.

If we're committed to using correct versioning for our crates, not opposed to using newtype!

[apfitzge]

lgtm - thanks for the detailed responses to my comments/questions!