fix: translateAddress should not rely on PublicKey constructor nam

[0xCryptoSheik]

Since anchor is used in web projects which will minify/mangle function
names, checking an input object for a custom constructor name is unsafe.

Moreover, checking the type of the input Address is unnecessary since
PublicKey's constructor handles:

• string
• array
• object with a { _bn } shape (like PublicKey itself)

For additional safety, a unit test should be added to prevent PublicKey
to stop supporting this unexpectedly.

See PublicKey implementation:

/**
 * JSON object representation of PublicKey class
 */
export type PublicKeyData = {
  /** @internal */
  _bn: BN;
};

function isPublicKeyData(value: PublicKeyInitData): value is PublicKeyData {
  return (value as PublicKeyData)._bn !== undefined;
}

export class PublicKey extends Struct {
  /** @internal */
  _bn: BN;

  /**
   * Create a new PublicKey object
   * @param value ed25519 public key as buffer or base-58 encoded string
   */
  constructor(value: PublicKeyInitData) {
    super({});
    if (isPublicKeyData(value)) {
      this._bn = value._bn;
    } else {
      if (typeof value === 'string') {
        // assume base 58 encoding by default
        const decoded = bs58.decode(value);
        if (decoded.length != 32) {
          throw new Error(`Invalid public key input`);
        }
        this._bn = new BN(decoded);
      } else {
        this._bn = new BN(value);
      }

      if (this._bn.byteLength() > 32) {
        throw new Error(`Invalid public key input`);
      }
    }
  }

[armaniferrante]

Thank you for these tests!

[paul-schaaf]

0xCryptoSheik we are now back to the failing test that led to his hacky implementation of translateAddress. Any idea why it works with that version but not yours?

i have added a check that just returns the input if it already has a ._bn which makes the lockup test pass but obv breaks yours. would ofc be better to find the root cause but if this makes the browser code work again, could release this first and then try to find out why just new PublicKey(address) isn't working (i've logged the inputs to the function in the lockup test but it just logs Pubkey so Im thinking there are maybe two versions of Pubkey?). im not that familiar with ts so idk how much finding the root cause would take.

[0xCryptoSheik]

Sorry @paul-schaaf I'm not sure I follow but I'll try.

This is the problem you describe in #1101 right, with my patch we're expecting that a PublicKey created before (where / how ?) after going through the translateAddress function, can still be seen as an instanceof PublicKey (where / how ?) ?

It's very well possible you have 2 different version of @solana/web3.js, and the part of the code which creates / verifies it uses a different version of PublicKey than the one used by translateAddress.

To check this, inside the package you can do:

yarn why @solana/web3.js

If you have a single version / hoisted, then it might not be the case of 2 different versions, or at least, not the simple case.

If the publicKey is generated / verified in a package that bundles @solana/web3.js then things are more complicated.

could release this first and then try to find out why just new PublicKey(address)

which version could we release first exactly?

Hope this makes sense & helps a bit, lmk.

[paul-schaaf]

@0xCryptoSheik

which version could we release first exactly?

actually, if we cannot find a fix for this soon, we can first roll back to the impl before #1098 and release that as part of 0.19.1

the thing is even if we have 2 different versions of Pubkey, I dont see how that should fail your tests. Ive logged the inputs to the translateAddress function in the lockup test and it's always a PublicKey with a ._bn, so given that our Pubkey can handle other objects that have a ._bn field, it shouldn't make the test fail even if it is a different class. but then again, the original impl of translateAddress does just let its input pass through if it's not a string which means that the cause of the failing test does seem to be that there are 2 different versions of Pubkey.

running yarn why @solana/web3.js gives me this:

➜  ts git:(fix/translate-address-no-constructor-name) ✗ yarn why @solana/web3.js
yarn why v1.22.10
[1/4] 🤔  Why do we have the module "@solana/web3.js"...?
[2/4] 🚚  Initialising dependency graph...
[3/4] 🔍  Finding dependency...
[4/4] 🚡  Calculating file sizes...
=> Found "@solana/web3.js@1.17.0"
info Has been hoisted to "@solana/web3.js"
info This module exists because it's specified in "dependencies".
info Disk size without dependencies: "7.44MB"
info Disk size with unique dependencies: "18.94MB"
info Disk size with transitive dependencies: "32.56MB"
info Number of shared dependencies: 27
✨  Done in 0.46s.

seems to be just one version

[vpontis]

I wonder if it would help to not bundle @solana/web3.js and have it as a thing that's used alongside of Anchor. This is kind of like how react doesn't bundle react-dom.

[paul-schaaf]

continued my debugging and it turns out it's not the PublicKeys that are of the wrong class that are the problem (those get converted to our PublicKey just fine) but it's our own Pubkeys that cause the problem. I haven't had time to look further but now one guess I have is that there is a comparison somewhere in our code base or solana's code base that uses === instead of eq. Because your implementation created a new public key every time, this === returns false and the tests fail. I would propose we now release this version that passes the tests and look for the === (or whatever it may be) bug in a separate issue. how does this sound @armaniferrante @0xCryptoSheik

[armaniferrante]

@paul-schaaf sgtm.

[0xCryptoSheik]

@paul-schaaf thank you for the explanation, sorry, I don't have time to follow-up as thoroughly as I'd like on this issue.
I'm happy with whatever you think is best.

[0xCryptoSheik]

I wonder if it would help to not bundle @solana/web3.js and have it as a thing that's used alongside of Anchor. This is kind of like how react doesn't bundle react-dom.

Not sure it'd change anything for this particular issue, but yes I believe it'd be best to have it as a peerDependency, it'd be easier for consumers who care to align versions so there aren't duplicates / versions mismatch.
Sad reality is that a lot of people ignore peerDependencies warnings or don't understand it. In any case, it'd be a breaking change.

[0xCryptoSheik]

@paul-schaaf, had to force-push to the branch to sign the commit

[michaelhly]

@armaniferrante can we please release a patch version of the anchor client that includes this commit?

[armaniferrante]

@armaniferrante can we please release a patch version of the anchor client that includes this commit?

Done v0.19.1-beta.1