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

feat(core): wallet provider updates #183

Merged
merged 9 commits into from
Jun 12, 2024
Merged

Conversation

drichar
Copy link
Collaborator

@drichar drichar commented Jun 8, 2024

Description

This PR makes multiple changes to all wallet providers in the core library. It represents many of the "must-haves" remaining before the 3.0.0 stable release. Several of these improvements/fixes depend on each other, which is why they appear in a single pull request:

Removes returnGroup

⚠️ BREAKING CHANGE - Apps that relied on the full original transaction group being returned with signed transactions inserted now must "re-assemble" the group manually instead.

Here is the new signTransactions type signature:

public signTransactions = async <T extends algosdk.Transaction[] | Uint8Array[]>(
  txnGroup: T | T[],
  indexesToSign?: number[]
): Promise<Uint8Array[]> => {
  // ...
}

Now signTransactions will ONLY return an array of signed transactions, instead of re-assembling and returning the original group by default.

MyAlgo Wallet's original implementation did not support signing with multiple connected accounts. The returnGroup logic facilitated a workaround where the group could be re-submitted to MyAlgo for each signer separately. It remained after MyAlgo was sunsetted because its removal is a breaking change.

Re-initialize client in signTransactions

Instead of throwing an error if the client is not initialized, the client is re-initialized. See #173

Removes duplicated logic in transactionSigner method

Here's what the entire method now looks like in every wallet (except Custom and Mnemonic):

public transactionSigner = async (
  txnGroup: algosdk.Transaction[],
  indexesToSign: number[]
): Promise<Uint8Array[]> => {
  return this.signTransactions(txnGroup, indexesToSign)
}

The transactionSigner method now simply calls signTransactions directly, so processing and signing logic doesn't need to be repeated. A single test has been added to each wallet that verifies signTransactions is called with the same arguments passed to it.

Refactors transaction processing

Processing for each transaction type (Transaction[] or Uint8Array[]) has been refactored into separate private methods: processTxns and processEncodedTxns, respectively.

  • Processing each type separately reduces a lot of the complexity from the previous implementation
  • Transaction types no longer need to be converted to byte arrays and then back again before being submitted for signature

Errors are always thrown

⚠️ BREAKING CHANGE - Consuming apps must now be prepared to catch and handle errors thrown by a wallet's connect and disconnect methods.

All public methods will throw their errors to the consuming application.

  • The connect method previously logged errors but returned a promise that resolved to an empty array. Now the promise is rejected with the error.
  • Similarly, if a wallet's disconnect or resumeSession methods contained any async function calls, errors would be caught and logged but the promise would resolve. Now errors are always thrown, rejecting the promise.
  • Apps will need to catch and handle these errors. The example apps will be updated to demonstrate this in a follow-up pull request.

Tests for signTransactions only check what gets passed to the wallet

All the library is responsible for is processing and normalizing what is passed to the wallet to be signed. The tests were previously mocking responses from the wallet and then verifying those responses were correct. (Hint: of course they are, they're being mocked!)

Now, given particular values for txnGroup and indexesToSign, the tests only assert that the array of formatted transactions sent to the wallet are correct.

Some wallets return an array containing null (or undefined) values for unsigned transactions (see ARC-0001). For these wallets, a mocked response with nullish values is returned by the wallet to make sure they are filtered out.

SVG icons

Several of the wallet icons contained extraneous SVG/XML markup that is auto-generated by tools such as Adobe Illustrator. When they are base64 encoded as strings, the resulting strings are significantly larger than they need to be.

I decoded each icon and made it as small as possible, only containing markup that is required. Then, rather than showing the encoded base64 string, the SVG markup itself is displayed inside a btoa function, so you can actually see what's there.

Mnemonic provider

These improvements probably could have been made in a separate PR, actually. While making the other changes to the Mnemonic provider, I also addressed some @todo: comments I left for myself:

  • Implemented persistToStorage option, so private key mnemonics wouldn't need to be entered into a prompt each time
  • Every method besides disconnect will abort if the active network detected is NetworkId.MAINNET
  • Added additional warnings to prevent misuse

Other changes

  • Better icons for KMD, Mnemonic, and Custom
  • Update store in disconnect method before any async actions (for snappier UX)
  • Debug logging statements use `[${this.metadata.name}]...` vs hard-coded '[ProviderName]...'

@drichar drichar added the v3 label Jun 11, 2024
@drichar drichar added this to the v3.0.0 stable release milestone Jun 11, 2024
@drichar drichar merged commit 1b7720a into v3 Jun 12, 2024
1 check passed
@drichar drichar deleted the feat/update-all-wallet-providers branch June 12, 2024 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant