Fixes ecdsa & custom signed extensions

leonardocustodio commented 9 months ago

Type

enhancement, bug_fix

Description

Refactored the extrinsic demo to support Westend and Astar networks, including the use of sr25519 and ecdsa wallets.
Enhanced multisig example with additional print statements for clarity.
Updated extrinsic and signing payload classes to support custom signed extensions.
Fixed ECDSA signing method to include the recovery byte and updated public key retrieval.
Updated dependencies to newer versions and added new chains configuration for polkadart.
Documented fixes in CHANGELOG for polkadart_keyring and updated the version to 0.4.2.

Changes walkthrough

Relevant files

Enhancement

7 files

extrinsic_demo.dart `Refactor extrinsic demo to support multiple networks and wallets` examples/bin/extrinsic_demo.dart Added imports for new modules and removed unused ones. Updated the provider and API initialization to use Westend instead of Polkadot. Added creation and usage of sr25519 and ecdsa wallets, including signing and submitting extrinsics. Implemented custom RPC call to fetch gas price from Astar network. Added handling for custom signed extensions with AssetHub.	+163/-77
multisig_example.dart `Enhance multisig example with print statements` packages/polkadart/example/multisig_example.dart Added print statements to indicate the execution of multisig operations.	+3/-0
abstract_payload.dart `Comment out unused method in abstract payload` packages/polkadart/lib/extrinsic/abstract_payload.dart - Commented out `maybeAssetIdEncoded` method.	+2/-3
extrinsic_payload.dart `Enhance extrinsic payload with custom signed extensions check` packages/polkadart/lib/extrinsic/extrinsic_payload.dart Added check for custom signed extensions in the registry. Commented out `assetId` in the encoded map.	+5/-1
asset_hub.dart `Comment out ChargeAssetTxPayment handling in AssetHub` packages/polkadart/lib/extrinsic/signed_extensions/asset_hub.dart - Commented out handling for `ChargeAssetTxPayment` signed extension.	+2/-2
signing_payload.dart `Enhance signing payload with custom signed extensions check` packages/polkadart/lib/extrinsic/signing_payload.dart Added check for custom signed extensions in the registry. Commented out `assetId` in the encoded map.	+5/-1
pubspec.yaml `Update dependencies and add new chains configuration` examples/pubspec.yaml Updated dependencies to newer versions. Added new chains configuration for polkadart.	+9/-7

Bug_fix

1 files

ecdsa.dart

Fix ECDSA signing and public key retrieval

packages/polkadart_keyring/lib/src/ecdsa.dart

Adjusted the signing method to include the recovery byte in the
signature.

Updated the public key getter to decode from the address.

+4/-3

Documentation

1 files

CHANGELOG.md

Document fixes in CHANGELOG for polkadart_keyring

packages/polkadart_keyring/CHANGELOG.md

Added entry for version 0.4.2 detailing fixes for ECDSA extrinsics
encoding.

+3/-0

Configuration changes

1 files

pubspec.yaml

Update polkadart_keyring version to 0.4.2

packages/polkadart_keyring/pubspec.yaml - Updated the version to 0.4.2.

+1/-1

✨ PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

github-actions[bot] commented 9 months ago

PR Description updated to latest commit (https://github.com/leonardocustodio/polkadart/commit/6d932ae6310353edf7c711092ae7061ec6e49cc6)

github-actions[bot] commented 9 months ago

PR Review

PR feedback
⏱️ Estimated effort to review [1-5]	4, because the PR introduces significant changes across multiple files, including enhancements, bug fixes, and dependency updates. The complexity of the changes, especially those related to cryptographic operations and custom signed extensions, requires careful review to ensure correctness and security.
🧪 Relevant tests	No
🔍 Possible issues	- The use of hard-coded mnemonics in `extrinsic_demo.dart` for wallet creation poses a security risk. It's advisable to use environment variables or secure storage solutions. - The `EcdsaKeyPair` class in `ecdsa.dart` modifies the signature by adding a recovery byte. This change could potentially affect signature verification processes if not handled correctly across all use cases. - The handling of custom signed extensions in `extrinsic/abstract_payload.dart`, `extrinsic/extrinsic_payload.dart`, and `extrinsic/signing_payload.dart` introduces complexity that could lead to errors if the registry is not properly managed or if the custom extensions are not correctly encoded. - Dependency updates in `pubspec.yaml` require thorough testing to ensure that the updated packages do not introduce breaking changes or new bugs.
🔒 Security concerns	Yes, because the PR includes changes to cryptographic operations and the handling of sensitive information such as private keys and mnemonics. Specifically, the addition of a recovery byte to the ECDSA signature and the use of hard-coded mnemonics for wallet creation require careful review to prevent security vulnerabilities.

Code feedback:

relevant file	examples/bin/extrinsic_demo.dart
suggestion	Consider using environment variables or a secure storage solution for mnemonics instead of hard-coding them in the source code. This change enhances security by preventing sensitive information from being exposed in the codebase. [important]
relevant line	"resource mirror lecture smooth midnight muffin position cup pepper fruit vanish also//10");

relevant file	packages/polkadart_keyring/lib/src/ecdsa.dart
suggestion	Ensure that the addition of the recovery byte to the ECDSA signature is compatible with all signature verification processes across the application. This might require additional testing or adjustments in the verification logic. [important]
relevant line	...signature.toCompactRawBytes(), signature.recovery ?? 0]);

relevant file	packages/polkadart/lib/extrinsic/abstract_payload.dart
suggestion	Review the commented-out `maybeAssetIdEncoded` method and related logic in `extrinsic_payload.dart` and `signing_payload.dart` to ensure that custom signed extensions are handled correctly. If this functionality is required, consider implementing it with proper error handling and validation. [medium]
relevant line	/* String maybeAssetIdEncoded(dynamic registry) {

relevant file	examples/pubspec.yaml
suggestion	After updating dependencies, perform thorough testing to ensure that the new versions are compatible with the existing codebase and do not introduce any regressions or new issues. [medium]
relevant line	polkadart: ^0.4.1

✨ Review tool usage guide:

**Overview:** The `review` tool scans the PR code changes, and generates a PR review. The tool can be triggered [automatically](https://github.com/Codium-ai/pr-agent/blob/main/Usage.md#github-app-automatic-tools) every time a new PR is opened, or can be invoked manually by commenting on any PR. When commenting, to edit [configurations](https://github.com/Codium-ai/pr-agent/blob/main/pr_agent/settings/configuration.toml#L19) related to the review tool (`pr_reviewer` section), use the following template: ``` /review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=... ``` With a [configuration file](https://github.com/Codium-ai/pr-agent/blob/main/Usage.md#working-with-github-app), use the following template: ``` [pr_reviewer] some_config1=... some_config2=... ```

Utilizing extra instructions

The `review` tool can be configured with extra instructions, which can be used to guide the model to a feedback tailored to the needs of your project. Be specific, clear, and concise in the instructions. With extra instructions, you are the prompter. Specify the relevant sub-tool, and the relevant aspects of the PR that you want to emphasize. Examples for extra instructions: ``` [pr_reviewer] # /review # extra_instructions=""" In the 'possible issues' section, emphasize the following: - Does the code logic cover relevant edge cases? - Is the code logic clear and easy to understand? - Is the code logic efficient? ... """ ``` Use triple quotes to write multi-line instructions. Use bullet points to make the instructions more readable.

How to enable\disable automation

- When you first install PR-Agent app, the [default mode](https://github.com/Codium-ai/pr-agent/blob/main/Usage.md#github-app-automatic-tools) for the `review` tool is: ``` pr_commands = ["/review", ...] ``` meaning the `review` tool will run automatically on every PR, with the default configuration. Edit this field to enable/disable the tool, or to change the used configurations

Auto-labels

The `review` tool can auto-generate two specific types of labels for a PR: - a `possible security issue` label, that detects possible [security issues](https://github.com/Codium-ai/pr-agent/blob/tr/user_description/pr_agent/settings/pr_reviewer_prompts.toml#L136) (`enable_review_labels_security` flag) - a `Review effort [1-5]: x` label, where x is the estimated effort to review the PR (`enable_review_labels_effort` flag)

Extra sub-tools

The `review` tool provides a collection of possible feedbacks about a PR. It is recommended to review the [possible options](https://github.com/Codium-ai/pr-agent/blob/main/docs/REVIEW.md#enabledisable-features), and choose the ones relevant for your use case. Some of the feature that are disabled by default are quite useful, and should be considered for enabling. For example: `require_score_review`, `require_soc2_ticket`, and more.

Auto-approve PRs

By invoking: ``` /review auto_approve ``` The tool will automatically approve the PR, and add a comment with the approval. To ensure safety, the auto-approval feature is disabled by default. To enable auto-approval, you need to actively set in a pre-defined configuration file the following: ``` [pr_reviewer] enable_auto_approval = true ``` (this specific flag cannot be set with a command line argument, only in the configuration file, committed to the repository) You can also enable auto-approval only if the PR meets certain requirements, such as that the `estimated_review_effort` is equal or below a certain threshold, by adjusting the flag: ``` [pr_reviewer] maximal_review_effort = 5 ```

More PR-Agent commands

> To invoke the PR-Agent, add a comment using one of the following commands: > - **/review**: Request a review of your Pull Request. > - **/describe**: Update the PR title and description based on the contents of the PR. > - **/improve [--extended]**: Suggest code improvements. Extended mode provides a higher quality feedback. > - **/ask \**: Ask a question about the PR. > - **/update_changelog**: Update the changelog based on the PR's contents. > - **/add_docs** 💎: Generate docstring for new components introduced in the PR. > - **/generate_labels** 💎: Generate labels for the PR based on the PR's contents. > - **/analyze** 💎: Automatically analyzes the PR, and presents changes walkthrough for each component. >See the [tools guide](https://github.com/Codium-ai/pr-agent/blob/main/docs/TOOLS_GUIDE.md) for more details. >To list the possible configuration parameters, add a **/config** comment.

See the [review usage](https://github.com/Codium-ai/pr-agent/blob/main/docs/REVIEW.md) page for a comprehensive guide on using this tool.

github-actions[bot] commented 9 months ago

PR Code Suggestions

Suggestions

security

Improve mnemonic handling for better security.

___ **Consider using a more secure way to handle mnemonics instead of hardcoding them in the
source code. This can be achieved by loading them from environment variables or secure
storage solutions.** [examples/bin/extrinsic_demo.dart [29-35]](https://github.com/leonardocustodio/polkadart/pull/429/files#diff-49aa83d51521da0e13e5069d8540d4a39fb08eb4c8378639aa262048bd2957d9R29-R35) ```diff -final sr25519Wallet = await KeyPair.sr25519.fromMnemonic( - "resource mirror lecture smooth midnight muffin position cup pepper fruit vanish also//10"); -final ecdsaWallet = await KeyPair.ecdsa.fromMnemonic( - "mushroom grab answer fork ripple virus swamp food excess old client arena//10"); +final sr25519Wallet = await KeyPair.sr25519.fromMnemonic(Environment.get('SR25519_MNEMONIC')); +final ecdsaWallet = await KeyPair.ecdsa.fromMnemonic(Environment.get('ECDSA_MNEMONIC')); ```

best practice

Add error handling for asynchronous operations.

___ **Use try-catch blocks around asynchronous calls that might throw exceptions, such as
network requests, to handle errors gracefully.** [examples/bin/extrinsic_demo.dart [45-48]](https://github.com/leonardocustodio/polkadart/pull/429/files#diff-49aa83d51521da0e13e5069d8540d4a39fb08eb4c8378639aa262048bd2957d9R45-R48) ```diff -final runtimeVersion = await stateApi.getRuntimeVersion(); -final block = await westProvider.send('chain_getBlock', []); +try { + final runtimeVersion = await stateApi.getRuntimeVersion(); + final block = await westProvider.send('chain_getBlock', []); +} catch (e) { + print('Error occurred: $e'); +} ```

Use a custom exception for better error context.

___ **Instead of throwing a generic exception, define a custom exception class that provides
more context about the error related to unsupported custom signed extensions.** [packages/polkadart/lib/extrinsic/extrinsic_payload.dart [63-65]](https://github.com/leonardocustodio/polkadart/pull/429/files#diff-2859cf016d711057af57ffcce8d535ed7f5ce89aa999eea4c6bd8f47a1b99fdbR63-R65) ```diff -throw Exception( +throw UnsupportedCustomSignedExtensionsException( 'Custom signed extensions are not supported on this registry. Please use registry from `runtimeMetadata.chainInfo.scaleCodec.registry`.'); ```

enhancement

Make lowS parameter configurable in sign method.

___ **Ensure that the sign method's lowS parameter is configurable by the caller, to provide
flexibility in signature generation.** [packages/polkadart_keyring/lib/src/ecdsa.dart [40]](https://github.com/leonardocustodio/polkadart/pull/429/files#diff-0e9548d76302d57b7580f75ddab13dd7139e84f2eaf997062dd18f49323c114aR40-R40) ```diff -final signature = _privateKey.sign(message, lowS: true); +final signature = _privateKey.sign(message, lowS: lowSParameter); ```

maintainability

Refactor payload signing to reduce code duplication.

___ **Refactor the code to avoid repeating the payload encoding and signing logic for different
wallet types by creating a function that accepts a KeyPair and returns the signed payload.** [examples/bin/extrinsic_demo.dart [76-98]](https://github.com/leonardocustodio/polkadart/pull/429/files#diff-49aa83d51521da0e13e5069d8540d4a39fb08eb4c8378639aa262048bd2957d9R76-R98) ```diff -final srPayload = payloadToSign.encode(westApi.registry); -final srSignature = sr25519Wallet.sign(srPayload); -final ecPayload = payloadToSign.encode(westApi.registry); -final ecSignature = ecdsaWallet.sign(ecPayload); +Uint8List signPayload(KeyPair wallet) { + final payload = payloadToSign.encode(westApi.registry); + return wallet.sign(payload); +} +final srSignature = signPayload(sr25519Wallet); +final ecSignature = signPayload(ecdsaWallet); ```

✨ Improve tool usage guide:

**Overview:** The `improve` tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered [automatically](https://github.com/Codium-ai/pr-agent/blob/main/Usage.md#github-app-automatic-tools) every time a new PR is opened, or can be invoked manually by commenting on a PR. When commenting, to edit [configurations](https://github.com/Codium-ai/pr-agent/blob/main/pr_agent/settings/configuration.toml#L69) related to the improve tool (`pr_code_suggestions` section), use the following template: ``` /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=... ``` With a [configuration file](https://github.com/Codium-ai/pr-agent/blob/main/Usage.md#working-with-github-app), use the following template: ``` [pr_code_suggestions] some_config1=... some_config2=... ```

Enabling\disabling automation

When you first install the app, the [default mode](https://github.com/Codium-ai/pr-agent/blob/main/Usage.md#github-app-automatic-tools) for the improve tool is: ``` pr_commands = ["/improve --pr_code_suggestions.summarize=true", ...] ``` meaning the `improve` tool will run automatically on every PR, with summarization enabled. Delete this line to disable the tool from running automatically.

Utilizing extra instructions

Extra instructions are very important for the `improve` tool, since they enable to guide the model to suggestions that are more relevant to the specific needs of the project. Be specific, clear, and concise in the instructions. With extra instructions, you are the prompter. Specify relevant aspects that you want the model to focus on. Examples for extra instructions: ``` [pr_code_suggestions] # /improve # extra_instructions=""" Emphasize the following aspects: - Does the code logic cover relevant edge cases? - Is the code logic clear and easy to understand? - Is the code logic efficient? ... """ ``` Use triple quotes to write multi-line instructions. Use bullet points to make the instructions more readable.

A note on code suggestions quality

- While the current AI for code is getting better and better (GPT-4), it's not flawless. Not all the suggestions will be perfect, and a user should not accept all of them automatically. - Suggestions are not meant to be simplistic. Instead, they aim to give deep feedback and raise questions, ideas and thoughts to the user, who can then use his judgment, experience, and understanding of the code base. - Recommended to use the 'extra_instructions' field to guide the model to suggestions that are more relevant to the specific needs of the project, or use the [custom suggestions :gem:](https://github.com/Codium-ai/pr-agent/blob/main/docs/CUSTOM_SUGGESTIONS.md) tool - With large PRs, best quality will be obtained by using 'improve --extended' mode.

More PR-Agent commands

> To invoke the PR-Agent, add a comment using one of the following commands: > - **/review**: Request a review of your Pull Request. > - **/describe**: Update the PR title and description based on the contents of the PR. > - **/improve [--extended]**: Suggest code improvements. Extended mode provides a higher quality feedback. > - **/ask \**: Ask a question about the PR. > - **/update_changelog**: Update the changelog based on the PR's contents. > - **/add_docs** 💎: Generate docstring for new components introduced in the PR. > - **/generate_labels** 💎: Generate labels for the PR based on the PR's contents. > - **/analyze** 💎: Automatically analyzes the PR, and presents changes walkthrough for each component. >See the [tools guide](https://github.com/Codium-ai/pr-agent/blob/main/docs/TOOLS_GUIDE.md) for more details. >To list the possible configuration parameters, add a **/config** comment.

See the [improve usage](https://github.com/Codium-ai/pr-agent/blob/main/docs/IMPROVE.md) page for a more comprehensive guide on using this tool.

leonardocustodio / polkadart