novasamatech / nova-wallet-android

Nova Wallet Android is a next gen application for Polkadot & Kusama ecosystem, transparent & community-oriented, focused on convenient UX/UI, fast performance & security.
https://novawallet.io
Apache License 2.0
50 stars 30 forks source link

Polkadot-API transactions make Nova-android crash #1493

Closed josepot closed 6 months ago

josepot commented 6 months ago

Attempting to sign a transaction with a DApp built with Polkadot-API (from within the DApps section) completely crashes the Nova android app.

AFAIK Polkadot-API is adhering to the SignerPayloadJSON defined by PJS and signing transactions with PAPI DApps works with all the other extensions that we have tested it with: talisman, polkadotjs, subwallet, polkagate and fearless.

It goes without saying that, from the polkadot-api team, we are more than willing to help at solving this issue.

If you need a DApp to reproduce this issue, I would recommend using Kheopswap, built by @0xKheops.

0xKheops commented 6 months ago

gm, actually kheopswap.eth.link url isn't good, it ignores updates despite the fact that it's an IPNS link..

Please use https://kheopswap.xyz

Would be great if you could look into this, thanks in advance.

valentunn commented 6 months ago

Hey @josepot @0xKheops ! I've checked the logs and it seems like dapp is not passing blockNumber field as a part of SignerPayloadJson:

{
  "id": "1714632049986.4",
  "msgType": "pub(extrinsic.sign)",
  "request": {
    "signedExtensions": [
      "CheckNonZeroSender",
      "CheckSpecVersion",
      "CheckTxVersion",
      "CheckGenesis",
      "CheckMortality",
      "CheckNonce",
      "CheckWeight",
      "ChargeAssetTxPayment"
    ],
    "specVersion": "0x0f4a10",
    "transactionVersion": "0x0e",
    "genesisHash": "0x68d56f15f85d3136970ec16946040bc1752654e906147f7e43e9d539d7c3de2f",
    "era": "0x00",
    "blockHash": "0x68d56f15f85d3136970ec16946040bc1752654e906147f7e43e9d539d7c3de2f",
    "nonce": "0x05",
    "address": "123aanTyAyypSRVxEyRGB6ywfJByZVxX6vknixn6PqASwiR8",
    "method": "0x28020837030801000002043205011f204df10500000000000000000000000041ff00000000000000000000000000002e33c5e14a53e874caa8e7c6d30bd20f6c51cda7dafaad1c465ca004fe61a63e010a03009efc1e1a32d5d9773f777c9f424d9edb5c89770fcb1f5b0505082463db61325c824f1200",
    "version": 4
  },
  "url": "https://kheopswap.xyz/#/swap",
  "origin": "dapp-request"
}

My guess is that is is related to dapp using Immortal era. By the way, It is unsafe to replay attacks so I highly recommend switching to mortal eras instead.

josepot commented 6 months ago

Hi @valentunn thank you so much for looking into this. You are absolutely correct about the fact that by default transactions should be created as mortal transactions. We actually noticed that yesterday while working on this and we were about to change that. So, thank you so much for pointing that out!

That being said, the DApp developer has the last word, and they can choose to create an inmortal transaction. My understanding is that in those cases the blockNumber is bogus, and thus it's not necessary to pass it in the payload. Nevertheless, if this fixes the issue and it doesn't cause problems anywhere else, then it's probably easier if we make that change in our end, and we always pass it no matter whether the transaction is mortal or inmortal 🙂.

valentunn commented 6 months ago

@josepot yes indeed in case of immortal era blockNumber is always equal to genesisHash so its effectively redundant. But it would be great to still have it for the sake of consistency and simplification of implementation especially considering that its not a default (and expected) case as you pointed out. Not sure how it works in PJS currently but we never experienced such an issue before though it might be because there is no pjs dapp that uses immortal txs

That said closing the issue for now with the consensus that PAPI will always supply blockNumber. Feel free to re-open in case there is a continuation