Skip to content

ts: better public key error msgs#1098

Merged
armaniferrante merged 4 commits into
otter-sec:masterfrom
paul-schaaf:ts__better_publickey_errormsgs
Dec 4, 2021
Merged

ts: better public key error msgs#1098
armaniferrante merged 4 commits into
otter-sec:masterfrom
paul-schaaf:ts__better_publickey_errormsgs

Conversation

@paul-schaaf

Copy link
Copy Markdown
Contributor

when you add a non publickey/string to the accounts object for an instruction you currently get this error.

Error: Non-base58 character

This PR changes the code so you get a more helpful error

Error: Wrong input type for account "authority" in the instruction accounts object for instruction "initialize". Expected PublicKey or string.

@paul-schaaf paul-schaaf marked this pull request as ready for review December 4, 2021 02:30
@paul-schaaf

Copy link
Copy Markdown
Contributor Author

my guess is that it fails because something that is neither a string nor a public key is passed into translateAddress. whatever it is can also be made into a pubkey but it's the wrong pubkey. will investigate later

@paul-schaaf

Copy link
Copy Markdown
Contributor Author

it works now, hacky solution but think this is good enough for now and we can try finding the source and fixing it in the other issue I linked

@armaniferrante armaniferrante merged commit 517838e into otter-sec:master Dec 4, 2021
@paul-schaaf paul-schaaf deleted the ts__better_publickey_errormsgs branch December 5, 2021 02:25
Comment thread ts/src/program/common.ts
const pk = new PublicKey(address);
return pk;
} else {
} else if (address.constructor.prototype.constructor.name === "PublicKey") {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This has introduced some bugs in our production build. I believe this is because JS is minifying the code and changing the name of constructors.

A couple suggestions to avoid this:

  • check if the method toBase58 exists on the object
  • add a name or type to the PublicKey constructor object similar to how Phantom adds isPhantom to window.solana

What do you think? Happy to open a PR that fixes this :)

cc @paul-schaaf

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

sorry for that, didnt consider this, my mistake!

fix is already on the way #1138

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Awesome, thanks for the pointer there @paul-schaaf :)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I don't have the full picture of the codebase and the issue, just wanted to chime in that some time ago I fixed precisely the same bug replacing such a clause with an instanceofaddress instanceof PublicKey.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

we can try again but it didnt work for us before #1101

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants