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

Support generic challenge signing. Introduce ABI deserialization. #361

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

0xmaayan
Copy link
Collaborator

@0xmaayan 0xmaayan commented Apr 10, 2024

Description

Following a community and a foundation request, adding support for generic challenge signing.

With the new generic challenge signing support this PR also introduces deserialization process with the types coming from a remote abi.
There are some limitation to the deserialization process

  1. vector<u8> can be a serialization representative to some types such as: public keys (ed25519, secp256k1, etc), string, etc. Since we can't really know anything about the value type other than it is a vector of u8, deserializing it is a bit challenging. Therefore, the sdk would try to deserialize it as a ed25519 public key type and if it fails it would return the array of u8 itself.
  2. Deserialization of Generic or Option types are not supported by the SDK as the deserializer class in the SDK can only handle a single class, i.e not a class with generics.

Future work

To not overload this work, will follow with next PRs to

  • remoteAbi.ts file needs to be refactored
  • deserialization flow to accept a local abi

Test Plan

added a new test for generic challenge
added tests for abi deserialization
tests are passing

Related Links

@0xmaayan 0xmaayan changed the title [WIP] Support general proof challenge [WIP] Support generic challenge signing Apr 10, 2024
@0xmaayan 0xmaayan force-pushed the proof_challenege branch 2 times, most recently from 9835f19 to 6be06b2 Compare April 11, 2024 12:08
@0xmaayan 0xmaayan marked this pull request as ready for review April 11, 2024 12:09
@0xmaayan 0xmaayan requested a review from a team as a code owner April 11, 2024 12:09
@0xmaayan 0xmaayan changed the title [WIP] Support generic challenge signing Support generic challenge signing Apr 11, 2024
@0xmaayan 0xmaayan force-pushed the proof_challenege branch 2 times, most recently from ce814c6 to 273ce91 Compare April 11, 2024 12:18
Copy link

@BriungRi BriungRi left a comment

Choose a reason for hiding this comment

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

Could the DevX be simpler here if createProofChallenge took two Account instead?

const account1 = new Account();
const account2 = new Account()
const proofChallenge = createProofChallenge({currentAccount: account1, newAccount: account2})

Seems like having to pass in all these different keys could be mistake-prone.

});

const challenge = await aptos.createProofChallenge({
struct: "0x1::account::RotationProofChallenge",

Choose a reason for hiding this comment

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

are there other structs that are valid here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

wdym? it is a generic implementation so any struct serves as a proof challenge is valid here


const response = await aptos.signAndSubmitTransaction({ signer: fromAccount, transaction });
const executedTransaction = await aptos.waitForTransaction({ transactionHash: response.hash });
expect(executedTransaction.success).toBeTruthy();

Choose a reason for hiding this comment

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

should we check if the on-chain public key has also changed?

@0xmaayan
Copy link
Collaborator Author

0xmaayan commented Apr 12, 2024

Could the DevX be simpler here if createProofChallenge took two Account instead?

const account1 = new Account();
const account2 = new Account()
const proofChallenge = createProofChallenge({currentAccount: account1, newAccount: account2})

Seems like having to pass in all these different keys could be mistake-prone.

It is very specific - not sure all proof challenges require 2 signatures. Also, it can be tricky from a wallet perspective when there is only one signer/wallet

@0xmaayan 0xmaayan force-pushed the proof_challenege branch 3 times, most recently from e1d32ca to 8d0fe99 Compare May 4, 2024 21:23
@0xmaayan 0xmaayan changed the title Support generic challenge signing Support generic challenge signing. Introduce ABI deserialization. May 5, 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