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

Allow Uint8Arrays public keys for kmd signing #549

Merged
merged 3 commits into from
May 2, 2022

Conversation

vividn
Copy link
Contributor

@vividn vividn commented Mar 23, 2022

The user gets the public key from algosdk.decodeAddress(authAddress) in the Uint8Array format, and the kmd client can already accept this how it is implemented. So it makes sense to allow this in the types for signTransactionWithSpecificPublicKey and signMultisigTransaction

The user gets the public key from `algosdk.decodeAddress(authAddress)` in the Uint8Array format, and the kmd client can already accept this how it is implemented. So it makes sense to allow this in the types for `signTransactionWithSpecificPublicKey` and `signMultisigTransaction`
@vividn vividn changed the title Allow Uint8Arrays type public keys when signing Allow Uint8Arrays public keys for kmd signing Mar 23, 2022
@CLAassistant
Copy link

CLAassistant commented Apr 28, 2022

CLA assistant check
All committers have signed the CLA.

@jasonpaulos
Copy link
Contributor

Sorry for the late reply. This looks like a good change, so if you can sign the CLA and pass our CI tests, I'll approve it.

@vividn
Copy link
Contributor Author

vividn commented Apr 28, 2022

Thanks for the response @jasonpaulos. It looks like the CI is failing because of a hash mismatch when downloading a docker image 🤔. is there a way to retrigger the CI? Maybe it will work if it is tried again

@jasonpaulos
Copy link
Contributor

I've rerun the failed tests, so hopefully it will pass now

@jasonpaulos
Copy link
Contributor

jasonpaulos commented Apr 28, 2022

@vividn actually the chrome test is still failing because this branch uses an older version of the chromedriver package. Could you merge the develop branch into here? That should fix it

@vividn
Copy link
Contributor Author

vividn commented Apr 28, 2022

My branch seems up to date with algorand:develop
image

@jasonpaulos
Copy link
Contributor

Interesting, you're right. I'll try to investigate further

@jasonpaulos
Copy link
Contributor

There was a problem in our develop branch which caused the test failure. Once #564 is merged (and we merge it into develop as well), could you again merge develop into this branch? Tests should pass once that happens

@vividn
Copy link
Contributor Author

vividn commented May 2, 2022

Sure I can do that. Thanks @jasonpaulos
Alternatively, I could change this PR base branch to be release/v1.16.0 and then get it merged in there. I notice there are already some corrections to types in client/kmd.ts, so it would fit well. What do you think?

@jasonpaulos
Copy link
Contributor

@vividn while that would work, we're already far along in the release and I don't want to delay it any longer. This will go out in the next release we make.

@vividn
Copy link
Contributor Author

vividn commented May 2, 2022

Success 😃

Copy link
Contributor

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

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

Great, thanks for your patience! 😅

@jasonpaulos jasonpaulos merged commit f48c5a6 into algorand:develop May 2, 2022
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