elixir-omg icon indicating copy to clipboard operation
elixir-omg copied to clipboard

More flexible struct-hash implementation

Open pnowosie opened this issue 4 years ago • 3 comments

This is a set of ideas that have emerged in #827, omisego/plasma-contracts#189 discussions. I'm compiling some of the comments to this issue.

Rationale

Abstract Layer Design has already changed and extended Transaction format. Also we gained some experience from initial EIP-712 implementation.

  • Transaction doesn't require padding in/outputs with placeholders/empty values
  • There can be more in/outputs than 4 and also less (e.g. deposit with no inputs and single output)
  • Make the metadata optional again (and do it right)
  • ...

To align with above we can leverage latest Metamask improvements (original message from @kevsul):

Support for dynamic arrays has just been added to the eth-sig-util package (🎉), which is what Metamask uses. The current Metamask (7.0.1) version doesn't support it, but it's been merged to master so next version will.

https://github.com/MetaMask/eth-sig-util/pull/54

This will allow us to specify transaction metadata as byte[] instead of byte32. The metadata field is still mandatory, but it can be empty, which I think is a better solution than having 2 different transaction types for with and without metadata.

Note also that this allows us to get rid of the input and output 'padding' that we currently do, and change the tx spec to something like this:

const txSpec = [
  { name: 'txType', type: 'uint256' },
  { name: 'inputs', type: 'Input[]' },
  { name: 'outputs', type: 'Output[]' },
  { name: 'metadata', type: 'byte[]' }
]

This won't work for current elixir-omg implementation because the contract code makes assumptions that the tx itself always has 4 inputs and 4 outputs, but I think we can use it in abstract layer.

If we require that the transaction itself has to have all of the above members, then we can avoid the 2 versions of the 'same' transaction with different identifiers as described in this issue.

This is tricky because unless we have a separate type for deposit tx (and this type is encoded in txbytes) this is indistinguishable from any other tx (blknum is not part of txbytes)

A deposit tx then is a tx with an empty inputs array, so it will be distinguishable.

pnowosie avatar Sep 18 '19 13:09 pnowosie

It looks like Metamask supports this now. Worth further investigation

kevsul avatar Nov 21 '19 08:11 kevsul

Is this still relevant @pnowosie @pdobacz ? If yes, please re-open!

InoMurko avatar Dec 18 '19 13:12 InoMurko

getting rid of the padding in EIP712 would be great, maintaining those padders is going to be a burden. Still relevant, reopening

pdobacz avatar Dec 18 '19 14:12 pdobacz