Skip to content
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

fix: make publicKey a required field in the TxMsgValue class. #659

Closed
wants to merge 1 commit into from

Conversation

zenodeapp
Copy link

@zenodeapp zenodeapp commented Mar 7, 2024

Hi again!

As I was working on #658 I couldn't seem to understand why the transactions weren't resolving. Checking the console of the extension I realized I didn't pass a publicKey value to my sendIbcTransfer causing the extension to panic! I thus made it a required field in the TxMsgValue class. This way it doesn't even get to the extension when a transaction doesn't contain this.

@zenodeapp
Copy link
Author

zenodeapp commented Mar 7, 2024

Okay, no wait. This does not match with the SDK. This expects an Option value.

Hmm. Is there a way to let the extension know that this is a required field without it panicking/crashing if it's not set? Or an alternative to making it required is to let the extension auto-read the public key from the signer if it's not set (if this is a smart thing to do).

Also, because we're halted now, I'm getting a similar panic-result. Is there a way to get these crashes to the frontend and not inside of the extension?

image

@zenodeapp
Copy link
Author

zenodeapp commented Mar 7, 2024

I'll keep this PR here to keep this open 'as a discussion/issue'.

@zenodeapp zenodeapp marked this pull request as draft March 7, 2024 21:14
@zenodeapp zenodeapp closed this Mar 9, 2024
@zenodeapp zenodeapp reopened this Mar 9, 2024
@mateuszjasiuk
Copy link
Collaborator

I'll keep this PR here to keep this open 'as a discussion/issue'.

Hello! Thank you for your c ontribution! Can you please close this one and open an issue? We will try to refine the "backlog" regularly and discuss potential problems there. Thanks! We should probably add contrib instructions to the README. We just have a lot of things to do atm.

@phy-chain
Copy link

@zenodeapp You need to push a message from the service worker, into the main window, then you'd be able to log the errors in the frontend. However, you'd want to enable that only in dev mode

@zenodeapp
Copy link
Author

I'll keep this PR here to keep this open 'as a discussion/issue'.

Hello! Thank you for your c ontribution! Can you please close this one and open an issue? We will try to refine the "backlog" regularly and discuss potential problems there. Thanks! We should probably add contrib instructions to the README. We just have a lot of things to do atm.

Yes I shall! This is probably not a big prio as I didn't realize it was only in development mode.

But @phy-chain that sounds like what needs to be done indeed. I'm not sure if it gets handled correctly in production mode either though, I'll test this later.

But yes will close this!

@zenodeapp zenodeapp closed this Mar 12, 2024
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.

3 participants