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

Additional comments/fixes #4

Open
DashKash54 opened this issue Feb 17, 2023 · 11 comments
Open

Additional comments/fixes #4

DashKash54 opened this issue Feb 17, 2023 · 11 comments

Comments

@DashKash54
Copy link

Adarsh from Lit Protocol. I've a few additional comments/fixes to discuss:

  1. Currently the BLS signature isn't handled while combining the signatures. It returns an error unlike in the JS SDK where it's handled
  2. How are you persisting the authSig, sessionKeys locally? So that the user only has to sign one time & then it's fetched from the storage. Like in the browser we're using the localStorage.
  3. executeJs() doesn't return decryptions, see the JS SDK
  4. Please add logging at "important" places like signatures, sessionKeys, etc. This helps simplify debugging. Kindly take a look at the JS SDK for "important" places where we log. For example: https://github.com/LIT-Protocol/lit-js-sdk/blob/serrano/src/utils/litNodeClient.js#L686
  5. signPKPTransaction & sendPKPTransaction should take an object as a param with exact property names: https://github.com/LIT-Protocol/lit-js-sdk/blob/serrano/src/utils/litNodeClient.js#L122. We do this to ensure that the params match with https://docs.ethers.org/v5/api/utils/transactions/
  6. We ideally don't want to pass the authSig to the signPKPTransaction & sendPKPTransaction functions as it's only required to be passed in the executeJs() & not really related to the "transaction" otherwise. Can do something similar to: https://github.com/LIT-Protocol/lit-js-sdk/blob/serrano/src/utils/litNodeClient.js#L167
  7. Any reason you increased the expiration time to 7 days? Lets keep the same default of 1 day: https://github.com/j-labs-xyz/lit-swift-sdk/blob/main/Lit_swift/Core/LitClient.swift#L87
  8. There's a difference between sessionExpirationKey & walletSig expiration: https://github.com/LIT-Protocol/lit-js-sdk/blob/serrano/src/utils/litNodeClient.js#L2234. But it uses the same value here
  9. Handling the BLS sigType also is missing here unlike the JS SDK
  10. Any reason why you're crafting the transaction in sendPKPTransaction instead of serializing the result of signPKPTransaction?
@LevenWin
Copy link
Collaborator

LevenWin commented Feb 18, 2023

Thanks for the detail feedbacks. I listed the todos I think need to be done

  • 3. return decryptions in executeJs()
  • 7. set the default expiration time to 1day
  • 8. use different expiration for sessionExpirationKey & walletSig

leave to next version?

  • 1 & 9. support BLS signature
  • 2. persist the authSig, sessionKeys locally
  • 4. add logging at "important" places
  • 5. take an object as a param in signPKPTransaction & sendPKPTransaction
  • 6. check and sign auth message in signPKPTransaction

@LevenWin
Copy link
Collaborator

  1. Any reason why you're crafting the transaction in sendPKPTransaction instead of serializing the result of signPKPTransaction?

Hi @Adarsh-Kumar28 I craft the signed transaction using the signature returned from signPKPTransaction , it does almost the same thing as const serializedTx = serialize(tx, signature);, I'm not sure if i was wrong

let signedTransactionModel = SignedTransaction(transaction: transactionModel, v: recid, r: r, s: s)

@LevenWin
Copy link
Collaborator

  1. How are you persisting the authSig, sessionKeys locally? So that the user only has to sign one time & then it's fetched from the storage. Like in the browser we're using the localStorage.

AuthSig is now cached locally in wallet example associated with google email. If cached in lit-swift-sdk,is there a way to get the signature corresponding to the google email? By the way, when should we delete the expired authSig?

@johnnynanjiang
Copy link
Member

Hi @Adarsh-Kumar28 , is there any specific reason for it? Do they have to be different?
8. use different expiration for sessionExpirationKey & walletSig

@DashKash54
Copy link
Author

AuthSig is now cached locally in wallet example associated with google email.

How about implementing it in the SDK itself so that this feature is available to anyone using the SDK.

If cached in lit-swift-sdk,is there a way to get the signature corresponding to the google email?

I guess this is what you have to implement. Something similar to how we store in the web browser: https://github.com/LIT-Protocol/lit-js-sdk/blob/serrano/src/utils/lit.js#L1541. You'll have to store it in the app of course.

By the way, when should we delete the expired authSig?

Lets keep it 24 hours

@DashKash54
Copy link
Author

Hi @Adarsh-Kumar28 I craft the signed transaction using the signature returned from signPKPTransaction , it does almost the same thing as const serializedTx = serialize(tx, signature);, I'm not sure if i was wrong

I was just curious if there's a serialize function like in the js-sdk that we can use. Anyways, if this is working we're good 🙂

@DashKash54
Copy link
Author

Hi @Adarsh-Kumar28 , is there any specific reason for it? Do they have to be different? 8. use different expiration for sessionExpirationKey & walletSig

Yeah that because we differentiate between AuthSig & sessionKey. SessionKeys are still in development & we plan to use it for batch signing hence smaller expiration

@DashKash54
Copy link
Author

Thanks for the detail feedbacks. I listed the todos I think need to be done

  • 3. return decryptions in executeJs()
  • 7. set the default expiration time to 1day
  • 8. use different expiration for sessionExpirationKey & walletSig

leave to next version?

  • 1 & 9. support BLS signature
  • 2. persist the authSig, sessionKeys locally
  • 4. add logging at "important" places
  • 5. take an object as a param in signPKPTransaction & sendPKPTransaction
  • 6. check and sign auth message in signPKPTransaction

Hi @LevenWin I've asked Debbie to confirm this

@LevenWin
Copy link
Collaborator

add logging at "important" places
Hi @Adarsh-Kumar28 , Logging have been supported #7

@LevenWin
Copy link
Collaborator

And I found a new issue about signature #6

@DashKash54
Copy link
Author

And I found a new issue about signature #6

Left a comment

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

No branches or pull requests

3 participants