Skip to content

Conversation

@zlayine
Copy link
Contributor

@zlayine zlayine commented May 5, 2025

PR Type

Bug fix


Description

  • Removed metadataHash computation logic

  • Deleted metadataHash field from payloadToSign

  • Deleted mode field from payloadToSign

  • Simplified SignerPayloadJSON construction


Changes walkthrough 📝

Relevant files
Bug fix
transaction.ts
Simplify signing payload properties                                           

resources/js/store/transaction.ts

  • Removed conditional metadataHash assignment
  • Removed metadataHash property from SignerPayloadJSON
  • Removed mode property from SignerPayloadJSON
  • Streamlined payloadToSign object creation
  • +0/-6     

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @zlayine zlayine requested a review from leonardocustodio May 5, 2025 16:17
    @zlayine zlayine self-assigned this May 5, 2025
    @github-actions
    Copy link

    github-actions bot commented May 5, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Missing Mode

    Verify that removing the mode field from the signing payload does not break compatibility with downstream signing logic or expected payload format.

    const payloadToSign: SignerPayloadJSON = {
    Hardcoded Version

    Confirm that the hardcoded version: 4 matches the chain's current Extrinsic version and will be kept in sync across runtime upgrades.

    version: 4,
    SignedExtensions Format

    Ensure the signedExtensions property from api.registry.signedExtensions serializes to the expected format in the payload for signing.

    signedExtensions: api.registry.signedExtensions,

    @github-actions
    Copy link

    github-actions bot commented May 5, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Re-add metadataHash field

    Reintroduce the metadataHash field to the signing payload to ensure compatibility
    with the expected payload schema. Default to the full zero hash when
    transaction.signingPayloadJson.metadataHash === '00'.

    resources/js/store/transaction.ts [60-61]

     tip: '0x00',
     version: 4,
    +metadataHash: transaction.signingPayloadJson.metadataHash === '00'
    +  ? '0x0000000000000000000000000000000000000000000000000000000000000000'
    +  : transaction.signingPayloadJson.metadataHash,
    Suggestion importance[1-10]: 8

    __

    Why: The metadataHash field was removed but is required by the SignerPayloadJSON schema for correct transaction signing; the suggestion accurately reintroduces the logic with the zero fallback.

    Medium
    Re-add mode field

    Add the mode property back into the signing payload so that the payload matches the
    original signing payload JSON structure and supports correct transaction mortality
    handling.

    resources/js/store/transaction.ts [60-61]

     tip: '0x00',
     version: 4,
    +mode: transaction.signingPayloadJson.mode,
    Suggestion importance[1-10]: 8

    __

    Why: The mode property is necessary for proper transaction mortality handling and was removed in the PR; restoring it ensures the payload matches the expected structure.

    Medium

    @zlayine zlayine merged commit 6299724 into master May 5, 2025
    4 checks passed
    @zlayine zlayine deleted the bugfix/PLA-2250/bad-signature branch May 5, 2025 16:20
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Development

    Successfully merging this pull request may close these issues.

    2 participants