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

Feature/lit 3140 make pkp base a private instance variable in pkp #509

Conversation

FedericoAmura
Copy link
Contributor

@FedericoAmura FedericoAmura commented Jun 19, 2024

Description

This PR cleans PKPBase authContext and its ex-extending clases PKPEthersWallet, PKPCosmosWallet and PKPSuiWallet. Including changes:

  • Removed extension from PKPBase, those classes now implement a shared interface and have a more restricted public interface
  • Make litNodeClient a required param in PKP wallets and remove fields to create it, delegating that responsibility to user
  • Removed redundant litNodeClient instance inside authContext
  • Made PKPBase final (with a private constructor, there is no final in TS)
  • Updated PKPClient due to these changes
  • Fixed several type definitions
  • Updated session sigs demo app to use all PKP wallet classes to test signing

This PR has BREAKING CHANGES due to changes in PKP*Wallet interfaces and removed things (WalletFactory)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Building and testing passing
  • New type validations SDK wide
  • session sigs demo updated to test signing using PKPEthersWallet, PKPCosmosWallet and PKPSuiWallet

Screenshot from 2024-06-21 13-09-42

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@FedericoAmura FedericoAmura self-assigned this Jun 21, 2024
@FedericoAmura FedericoAmura marked this pull request as ready for review June 21, 2024 13:04
Copy link
Collaborator

@Ansonhkg Ansonhkg left a comment

Choose a reason for hiding this comment

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

Nice! I like that fact that we are passing the pkpBase as an instance now instead of extending it, and also making litNodeClient a required param so we stop fiddling around the default configuration underneath.

@Ansonhkg Ansonhkg self-requested a review June 21, 2024 16:03
Copy link
Collaborator

@Ansonhkg Ansonhkg left a comment

Choose a reason for hiding this comment

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

Ran all tests using Tinny and all passed! Approved!

Command ran:
NETWORK=cayenne yarn test:local --filter=testPkpEthers

- testPkpEthersWithEoaSessionSigsToSignWithAuthContext (Passed in 33405.67 ms)
- testPkpEthersWithEoaSessionSigsToSignMessage (Passed in 37533.46 ms)
- testPkpEthersWithEoaSessionSigsToEthSign (Passed in 34069.55 ms)
- testPkpEthersWithEoaSessionSigsToPersonalSign (Passed in 37605.22 ms)
- testPkpEthersWithEoaSessionSigsToSendTx (Passed in 29976.41 ms)
- testPkpEthersWithEoaSessionSigsToEthSignTransaction (Passed in 33044.19 ms)
- testPkpEthersWithEoaSessionSigsToEthSignTypedDataV1 (Passed in 24990.92 ms)
- testPkpEthersWithEoaSessionSigsToEthSignTypedDataV3 (Passed in 42685.83 ms)
- testPkpEthersWithEoaSessionSigsToEthSignTypedDataV4 (Passed in 38330.83 ms)
- testPkpEthersWithEoaSessionSigsToEthSignTypedData (Passed in 48618.72 ms)
- testPkpEthersWithEoaSessionSigsToEthSignTypedDataUtil (Passed in 42726.87 ms)
- testPkpEthersWithPkpSessionSigsToSignMessage (Passed in 34424.74 ms)
- testPkpEthersWithPkpSessionSigsToEthSign (Passed in 33375.91 ms)
- testPkpEthersWithPkpSessionSigsToPersonalSign (Passed in 29709.57 ms)
- testPkpEthersWithPkpSessionSigsToSendTx (Passed in 38705.77 ms)
- testPkpEthersWithPkpSessionSigsToEthSignTransaction (Passed in 34712.24 ms)
- testPkpEthersWithPkpSessionSigsToEthSignTypedDataV1 (Passed in 30473.60 ms)
- testPkpEthersWithPkpSessionSigsToEthSignTypedDataV3 (Passed in 40823.64 ms)
- testPkpEthersWithPkpSessionSigsToEthSignTypedDataV4 (Passed in 37509.25 ms)
- testPkpEthersWithPkpSessionSigsToEthSignTypedData (Passed in 45355.16 ms)
- testPkpEthersWithPkpSessionSigsToEthSignTypedDataUtil (Passed in 51054.24 ms)
- testPkpEthersWithLitActionSessionSigsToSignMessage (Passed in 37804.52 ms)
- testPkpEthersWithLitActionSessionSigsToEthSign (Passed in 35620.42 ms)
- testPkpEthersWithLitActionSessionSigsToPersonalSign (Passed in 35333.73 ms)
- testPkpEthersWithLitActionSessionSigsToSendTx (Passed in 29745.38 ms)
- testPkpEthersWithLitActionSessionSigsToEthSignTransaction (Passed in 42488.54 ms)
- testPkpEthersWithLitActionSessionSigsToEthSignTypedDataV1 (Passed in 30233.94 ms)
- testPkpEthersWithLitActionSessionSigsToEthSignTypedDataV3 (Passed in 44792.96 ms)
- testPkpEthersWithLitActionSessionSigsToEthSignTypedDataV4 (Passed in 26830.14 ms)
- testPkpEthersWithLitActionSessionSigsToEthSignTypedData (Passed in 49076.07 ms)
- testPkpEthersWithLitActionSessionSigsToEthSignTypedDataUtil (Passed in 40867.13 ms)

Copy link
Collaborator

@joshLong145 joshLong145 left a comment

Choose a reason for hiding this comment

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

🔥

packages/pkp-ethers/src/lib/helper.ts Show resolved Hide resolved
@Ansonhkg Ansonhkg merged commit d4b488b into master Jul 8, 2024
4 checks passed
@Ansonhkg Ansonhkg deleted the feature/lit-3140-make-pkp-base-a-private-instance-variable-in-pkp branch July 8, 2024 14:39
Copy link
Contributor

@MaximusHaximus MaximusHaximus left a comment

Choose a reason for hiding this comment

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

👍 This is awesome dude, nice work.

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