-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ts: better public key error msgs #1098
ts: better public key error msgs #1098
Conversation
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 |
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 |
@@ -58,8 +58,10 @@ export function translateAddress(address: Address): PublicKey { | |||
if (typeof address === "string") { | |||
const pk = new PublicKey(address); | |||
return pk; | |||
} else { | |||
} else if (address.constructor.prototype.constructor.name === "PublicKey") { |
There was a problem hiding this comment.
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
ortype
to thePublicKey
constructor object similar to how Phantom addsisPhantom
towindow.solana
What do you think? Happy to open a PR that fixes this :)
cc @paul-schaaf
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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 instanceof
— address instanceof PublicKey
.
There was a problem hiding this comment.
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
when you add a non publickey/string to the accounts object for an instruction you currently get this error.
This PR changes the code so you get a more helpful error