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

Add all signers to the Transaction when serializing with ToStackItem #2461

Closed
wants to merge 5 commits into from

Conversation

shargon
Copy link
Member

@shargon shargon commented May 6, 2021

Close #2460

@shargon shargon requested a review from erikzhang May 6, 2021 21:29
@cloud8little
Copy link
Contributor

Testing

@cloud8little
Copy link
Contributor

cloud8little commented May 7, 2021

Since the Signers is converted to ByteString, how can it be converted back to Signer type in the contract? The fourth result of Array type is the returned Signers. As @csmuller mentioned, I think the scripthash of the signers accounts is enough. Besides, I prefer to reserve the original Sender field, which can save a lot of time to make existing contract compatible when they use tx.Sender a lot.

{
  "jsonrpc": "2.0",
  "id": 1,
  "result": {
    "script": "wh8MC2NoZWNrU2lnbmVyDBRTmj3IXRZ1Tf+gBpCEMh4YG3SmkEFifVtS",
    "state": "HALT",
    "gasconsumed": "1050210",
    "exception": null,
    "stack": [
      {
        "type": "Array",
        "value": [
          {
            "type": "ByteString",
            "value": "qO1jTXO3f137AmS91H3MSookn8NFTdPtqgaURWsdf68="
          },
          {
            "type": "Integer",
            "value": "0"
          },
          {
            "type": "Integer",
            "value": "0"
          },
          {
            "type": "Array",
            "value": [
              {
                "type": "ByteString",
                "value": "fu4aq+tn7R15HUTk9fzzrpFxqHEQBPlbZ8nQL4JCkov3isQhmpO9EP21fj0kMR6lYXRJjn3DJQem7XroYNUBXdiaWwTnWPtnw6I+BU6v20QSknqeDm1/DzmQMR8r4GiZ90DEZznR"
              },
              {
                "type": "ByteString",
                "value": "Edm1mjSxnQkQ+yCRGOv8WYg6rvwQBPlbZ8nQL4JCkov3isQhmpO9EP21fj0kMR6lYXRJjn3DJQem7XroYNUBXdiaWwTnWPtnw6I+BU6v20QSknqeDm1/DzmQMR8r4GiZ90DEZznR"
              }
            ]
          },
          {
            "type": "Integer",
            "value": "0"
          },
          {
            "type": "Integer",
            "value": "0"
          },
          {
            "type": "Integer",
            "value": "0"
          },
          {
            "type": "ByteString",
            "value": ""
          }
        ]
      }
    ]
  }
}

@superboyiii
Copy link
Member

We should avoid any incompatible changes on RC3 only if it's very critical.

@shargon
Copy link
Member Author

shargon commented May 7, 2021

The option B it's serialize Signers at the end, I think that this option it's most optimal, what do you prefer @erikzhang ?

@erikzhang
Copy link
Member

The option B it's serialize Signers at the end

I think it's okay. But why do we need this?

@shargon
Copy link
Member Author

shargon commented May 9, 2021

I think it's okay. But why do we need this?

SC maybe will need to know all the signers

@@ -421,6 +421,7 @@ public StackItem ToStackItem(ReferenceCounter referenceCounter)
NetworkFee,
ValidUntilBlock,
Script,
new Array(referenceCounter, Signers.Select(u=> (ByteString)u.ToArray()))
Copy link
Member

Choose a reason for hiding this comment

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

How to deserialize it in a contract?

Copy link
Member Author

Choose a reason for hiding this comment

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

Only the address better?

Copy link
Member

Choose a reason for hiding this comment

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

@csmuller Does it work for you?

Copy link
Contributor

Choose a reason for hiding this comment

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

Think of contract-sponsored transactions that rely on contract's verify method. I want to see signers there, but I'd also like to see their witness scopes so that I could check them. Otherwise user can send {{0xCONTRACT, Global}, {0xUSER, Whatever}} transaction and if contract approves it then transaction could steal some assets from it.

Or maybe it can be generalized, even if contract is not a sender it still should be worried about its witness scope.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should answer the question first: why do we need to read the signers in a contract?

Copy link
Contributor

Choose a reason for hiding this comment

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

To implement some checks in verify?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can create some syscalls to handle it? Just like CheckWitness?

@shargon shargon closed this Nov 8, 2021
@shargon shargon deleted the signers-instead-of-sender branch November 8, 2021 12:47
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.

Add all signers to the Transaction when serializing with ToStackItem
5 participants