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

refactor(experimental): create a sham for PublicKey #1861

Merged

Conversation

steveluscher
Copy link
Contributor

@steveluscher steveluscher commented Nov 17, 2023

Preamble

Imagine that you depend on a module that itself depends on the legacy @solana/web3.js. This makes it difficult for you to remove the legacy library from your project.

The chances are, though, that module only needs a fraction of what web3.js provides.

The goal of the sham – of which this PR is the first – is to provide drop-in replacements for just the things that a set list of known dependencies require. You should be able to hot-swap the sham in via npm overrides or yarn resolutions and thereby drop the legacy web3.js from your bundle.

Summary

This PR introduces an interface compatible(-ish) PublicKey that you can use as a drop-in replacement for the one in @solana/web3.js@1

Test Plan

cd packages/library-legacy-sham/
pnpm test:unit:browser
pnpm test:unit:node

See later diffs for a functional test plan where we actually drop the sham in to an actual application and see the results.

Addresses #1825.

@steveluscher steveluscher force-pushed the 11-15-refactor_experimental_delete_Buffer_in_non-Node_tests branch from aa45e91 to e3abdd2 Compare November 17, 2023 19:45
@steveluscher steveluscher force-pushed the 11-15-refactor_experimental_create_a_sham_for_PublicKey_ branch from 8036a03 to f9982dd Compare November 17, 2023 19:45
Copy link
Contributor

@lorisleiva lorisleiva left a comment

Choose a reason for hiding this comment

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

Love it! ❤️


This package is a drop-in replacement for a _subset_ of the version `1.x` `@solana/web3.js` interfaces. Its goal is to satisfy the legacy interfaces with as little code as possible.

If you depend on on web3.js _directly_ then you should not use this. Instead, migrate to version `>=2` of `@solana/web3.js`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
If you depend on on web3.js _directly_ then you should not use this. Instead, migrate to version `>=2` of `@solana/web3.js`.
If you depend on web3.js _directly_ then you should not use this. Instead, migrate to version `>=2` of `@solana/web3.js`.

Comment on lines +22 to +23
'findProgramAddress',
'findProgramAddressSync',
Copy link
Contributor

Choose a reason for hiding this comment

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

I expect people are likely gonna complain about these not being shamed.

Copy link
Contributor

Choose a reason for hiding this comment

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

🙅🏼 shame!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

People won't use the sham. Libraries will use the sham (eg. @metaplex/mpl-token-metadata). So long as we support everything the libraries need, we're done.

it('returns `true` given two public keys with the same address', () => {
expect(publicKey.equals(publicKey)).toBe(true);
});
it('returns `false` given two public keys with the same address', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
it('returns `false` given two public keys with the same address', () => {
it('returns `false` given two public keys with different addresses', () => {

});
describe('the `toJSON()` method', () => {
it('returns the `Address` related to the public key as a string', () => {
expect(publicKey.toJSON()).toBe(VALID_ADDRESS);
Copy link
Contributor

Choose a reason for hiding this comment

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

I may be wrong here but shouldn't toJson() return a JSON string instead of just a string? I.e. "I'm valid JSON" instead of I'm not.

Suggested change
expect(publicKey.toJSON()).toBe(VALID_ADDRESS);
expect(publicKey.toJSON()).toBe(`"${VALID_ADDRESS}"`);

Copy link
Contributor

Choose a reason for hiding this comment

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

This confused me too in something else where I was using JSON.stringify(..) but it seems like JavaScript ignores the quotes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see, toJSON doesn't return you a JSON string? Just the JSON-compatible JavaScript object?

Copy link
Contributor

@buffalojoec buffalojoec Nov 20, 2023

Choose a reason for hiding this comment

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

The more I think about it the less sure I am, but I'm imagining it's a JSON string until you pass it into a JavaScript function, then they are clipped? 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm just matching the behaviour of the existing library without trying to use my brain. I might hurt myself otherwise.

Copy link
Contributor

@mcintyre94 mcintyre94 left a comment

Choose a reason for hiding this comment

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

This looks like a neat approach! Are there any particular libraries you're using to figure out what is/isn't required in the sham?

Copy link
Contributor

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

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

Are we really naming it legacy-sham? 🤭

Also PSA to all README'ers: Don't use instanceof or you're toast.

describe('the `toBuffer()` method', () => {
if (__NODEJS__) {
it('returns a buffer representing the public key bytes', () => {
expect(publicKey.toBuffer()).toEqual(Buffer.from(VALID_ADDRESS_BYTES));
Copy link
Contributor

Choose a reason for hiding this comment

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

💙

@steveluscher steveluscher changed the base branch from 11-15-refactor_experimental_delete_Buffer_in_non-Node_tests to master November 20, 2023 21:46
@steveluscher steveluscher force-pushed the 11-15-refactor_experimental_create_a_sham_for_PublicKey_ branch from f9982dd to 516772f Compare November 20, 2023 21:46
@steveluscher
Copy link
Contributor Author

steveluscher commented Nov 20, 2023

Merge activity

@steveluscher steveluscher merged commit 0983bae into master Nov 20, 2023
7 checks passed
@steveluscher steveluscher deleted the 11-15-refactor_experimental_create_a_sham_for_PublicKey_ branch November 20, 2023 21:52
@steveluscher
Copy link
Contributor Author

Are there any particular libraries you're using to figure out what is/isn't required in the sham?

Yeah; starting with @solana/wallet-adapter and then I'll move on to @orca-so/*-sdk then to @coral-xyz/anchor then @metaplex-foundation/mpl-token-metadata then then then until some apps actually start working.

Copy link
Contributor

github-actions bot commented Dec 5, 2023

Because there has been no activity on this PR for 14 days since it was merged, it has been automatically locked. Please open a new issue if it requires a follow up.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants