Skip to content

[Proposal] Show CS address in getrawtransaction output#784

Merged
zeptin merged 3 commits intostratisproject:release/1.1.1.0from
zeptin:coldstaking-20211126
Nov 29, 2021
Merged

[Proposal] Show CS address in getrawtransaction output#784
zeptin merged 3 commits intostratisproject:release/1.1.1.0from
zeptin:coldstaking-20211126

Conversation

@zeptin
Copy link
Collaborator

@zeptin zeptin commented Nov 26, 2021

The rationale for this is in the code comments; essentially this is to improve the user experience for block explorer users trying to check their cold staking balances or staking activity.

After picking a cold staking transaction ID at random, the modified getrawtransaction output looks like this:

    {
      "value": 780.5398,
      "n": 1,
      "scriptPubKey": {
        "asm": "OP_DUP OP_HASH160 OP_ROT OP_IF OP_CHECKCOLDSTAKEVERIFY 98f869eb17b7d9ed5529bb8d139d960624ab7c39 OP_ELSE d8e6de0c675ad75f430ccbd08f3a8b0e519dc6ec OP_ENDIF OP_EQUALVERIFY OP_CHECKSIG",
        "hex": "76a97b63b91498f869eb17b7d9ed5529bb8d139d960624ab7c396714d8e6de0c675ad75f430ccbd08f3a8b0e519dc6ec6888ac",
        "reqSigs": 1,
        "type": "coldstaking",
        "addresses": [
          "Xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
        ]
      }
    }

@zeptin zeptin requested a review from fassadlr November 26, 2021 01:11
@zeptin zeptin requested a review from quantumagi November 26, 2021 08:26
Copy link
Contributor

@quantumagi quantumagi left a comment

Choose a reason for hiding this comment

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

LGTM. My only concern is that we should probably not be showing the pubkeys that make up the cold staking script to the user as normal addresses at all - to prevent them from being used for spending funds towards. This represents a move away from that potential goal.

@zeptin
Copy link
Collaborator Author

zeptin commented Nov 29, 2021

LGTM. My only concern is that we should probably not be showing the pubkeys that make up the cold staking script to the user as normal addresses at all - to prevent them from being used for spending funds towards. This represents a move away from that potential goal.

In the interests of making as few changes at a time as possible, perhaps this should go in as-is, but we could indeed change the version byte for cold staking addresses in future. This would require changes to the cold staking controller and possibly the wallet UI too. There may be other places in the codebase that would need to be made aware of a new address type.

Either that or make the display of the addresses purely cosmetic output with no recognition of the modified addresses in the codebase. This could have its own drawbacks though, as the API/RPC would be outputting 'addresses' that it can't then give any further information about.

@zeptin zeptin merged commit 2df7cee into stratisproject:release/1.1.1.0 Nov 29, 2021
@zeptin zeptin deleted the coldstaking-20211126 branch November 29, 2021 11:07
zeptin added a commit to zeptin/StratisFullNode that referenced this pull request Nov 29, 2021
…t#784)

* Show CS address in getrawtransaction output

* Move type determination

* Update src/Stratis.Bitcoin/Controllers/Models/TransactionModel.cs
zeptin added a commit that referenced this pull request Nov 29, 2021
* Show CS address in getrawtransaction output

* Move type determination

* Update src/Stratis.Bitcoin/Controllers/Models/TransactionModel.cs
rowandh pushed a commit that referenced this pull request Dec 2, 2021
* Show CS address in getrawtransaction output

* Move type determination

* Update src/Stratis.Bitcoin/Controllers/Models/TransactionModel.cs
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.

2 participants