-
Notifications
You must be signed in to change notification settings - Fork 87
feat: Allow PublicKey for TokenCreateTransaction keys #754
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
base: main
Are you sure you want to change the base?
feat: Allow PublicKey for TokenCreateTransaction keys #754
Conversation
a9716f3 to
e4adf03
Compare
exploreriii
left a comment
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.
Hi @Adityarya11
Good efforts with this, and lots of good content
Some of the tests are using unusual methods that sometimes don't test what you are intending -- this is normal, being fairly new to the sdk, but building knowledge will help improve your accuracy over time
I'd recommend checking with us @nadineloepfe @exploreriii @manishdait and see who may be available over the next few days
Sorry for lots of confusion in my comments and imports, i am refactoring my code again and will fix ASAP. |
e4adf03 to
cc4e84f
Compare
|
@exploreriii can you please approve the test workflow so that i can check whether tests are failing or not... |
|
@manishdait fixing all the issues ASAP |
cc4e84f to
ce1b7a1
Compare
|
Hello @exploreriii, Additionally, when either you or @manishdait have a moment, I would appreciate a review. Thanks |
|
Yes, thank you for the reminder @Adityarya11 |
exploreriii
left a comment
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.
Hi @Adityarya11
Well done - thank you for taking the time to get the details
I think this is great
I will pass on to @nadineloepfe or @manishdait for a second opinion as keys are sensitive
exploreriii
left a comment
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.
also, please rebase and change this line
- feat: Allow
PrivateKeyto be used for keys inTopicCreateTransactionfor consistency. - in your changelog
to the new unreleased section as we have released 0.1.8 now
ce1b7a1 to
622293f
Compare
Signed-off-by: Adityarya11 <arya050411@gmail.com>
622293f to
08d4faa
Compare
|
Requesting second opinion @nadineloepfe or @manishdait for as keys are sensitive |
Description
This PR enhances
TokenCreateTransactionto accept bothPublicKeyandPrivateKeyobjects for all token key fields (admin, supply, freeze, wipe, etc.), enabling non-custodial transaction construction.With this change, an application (e.g., an automated agent) can build a transaction using only a user’s PublicKey, serialize it to bytes, and then return it for the user to sign locally with their PrivateKey — preserving ownership and key isolation.
This implementation follows the pattern used in the TypeScript SDK and aligns with
TopicCreateTransaction.pybehavior, as previously suggested by the maintainer.Key Changes
Introduced
Key = Union[PrivateKey, PublicKey]type alias for flexibility and backward compatibility.Updated the
TokenKeysdataclass and allset_*_key()methods to accept the newKeytype.Re-implemented
_to_proto_key()to:PrivateKeyvia.public_key();TypeErrorfor unsupported key types.Added comprehensive unit and integration tests to ensure safety and parity:
Security & Safety
PrivateKeyandPublicKeyby Python class type, not by raw bytes — avoiding ambiguity where ED25519 key bytes might appear identical.Compatibility
PrivateKeycontinues to work.PublicKeyfor non-custodial scenarios.Example:
Parity with TypeScript SDK
Behavior now matches the JS SDK:
PrivateKeyorPublicKey)Related Issue
Fixes #735
Checklist
ED25519,ECDSA, and non-custodial flow)CHANGELOG.md