Add support for meta/signed transactions #164

lleifermann · 2022-08-17T09:01:05Z

This adds support for the EIP1056 "signed" functions for meta transactions. Enabling users of this dependencies to carry out transactions on behalf of someone else's wallet, as long as the signer of the transaction can prove he has access to the private key material of the current owner of the identity.

We've added tests for each meta transaction case actually resolving the did document afterwards. (Huge kudos to the whole test suite. It really enabled us to understand all of this fast.)

Some open questions we still have:

We added a helper function signMetaTxData to sign the data in the correct format for the tests to work. I think this is not normally something this repository really needs thats why we kept it in the dev deps. Is this OK?
Added an Interface MetaSignature to keep the signed method variants input parameters clean. Should this be something this package also exports for others to use?

src/__tests__/resolver.test.ts

lleifermann · 2022-08-17T10:02:24Z

@mirceanis

Just FYI we are still discussing right now if this repository shouldn't export hashing functions for each of the signed methods. Like hashSetAttributeSigned as this is a very contract specific thing on how this has to be hashed.
(This would move the the ethers dev dep to a normal dep though)

TBH i think the best way would be if the contract actually exposed some functions and you let the hashing (before signing) happen via the contract. But as changing the contract is a rather hard way to pull off we think locating this as close as possible to the contract interaction itself might make sense.

This would also enable us in the future that the contract owns this logic. WDYT?

cc @strumswell @dennisvonderbey

mirceanis

Wonderful initiative here, thanks for contributing this.

Please see my notes about the dependencies, but especially about the nonce tracking and tell me what you think too.

package.json

src/__tests__/resolver.test.ts

src/helpers.ts

mirceanis · 2022-08-17T10:10:48Z

src/helpers.ts

+  dataBytes: Uint8Array,
+  didReg: Contract
+) {
+  const nonce = await didReg.nonce(signerAddress)


This method should be tested against the existing deployments because there are differences between the older contract (0xdca7ef03e98e0dc2b855be647c39abe984fcf21b) and the latest one (for example 0x63eD58B671EeD12Bc1652845ba5b2CDfBff198e0) regarding nonce tracking.

See uport-project/ethr-did-registry#12

This issue in particular makes me want to move to a new contract.

So is this about staying compatible to different versions of the contract?

Yes. It's about compatibility.
The older contract, which is deployed to mainnet and ethereum testnets tracks the nonce as a property of the owner, and not as a property of the signer.
Most of the time these are the same address, until the owner of the DID is changed, at which point this becomes an issue.

src/helpers.ts

mirceanis · 2022-08-17T10:24:40Z

@mirceanis

Just FYI we are still discussing right now if this repository shouldn't export hashing functions for each of the signed methods. Like hashSetAttributeSigned as this is a very contract specific thing on how this has to be hashed. (This would move the the ethers dev dep to a normal dep though)

TBH i think the best way would be if the contract actually exposed some functions and you let the hashing (before signing) happen via the contract. But as changing the contract is a rather hard way to pull off we think locating this as close as possible to the contract interaction itself might make sense.

This would also enable us in the future that the contract owns this logic. WDYT?

cc @strumswell @dennisvonderbey

This could also be achieved using a wrapper contract that exposes some pure functions for hashing. But I'm not sure I understand what the difference would be between having the contract do the hashing or performing the hashing off-chain.
Is there something else than keccak256 natively available for hashing on EVM ? Or have I misunderstood your ask here?

lleifermann · 2022-08-17T10:52:17Z

@mirceanis
Just FYI we are still discussing right now if this repository shouldn't export hashing functions for each of the signed methods. Like hashSetAttributeSigned as this is a very contract specific thing on how this has to be hashed. (This would move the the ethers dev dep to a normal dep though)
TBH i think the best way would be if the contract actually exposed some functions and you let the hashing (before signing) happen via the contract. But as changing the contract is a rather hard way to pull off we think locating this as close as possible to the contract interaction itself might make sense.
This would also enable us in the future that the contract owns this logic. WDYT?
cc @strumswell @dennisvonderbey

This could also be achieved using a wrapper contract that exposes some pure functions for hashing. But I'm not sure I understand what the difference would be between having the contract do the hashing or performing the hashing off-chain. Is there something else than keccak256 natively available for hashing on EVM ? Or have I misunderstood your ask here?

This is more of a code style and accessibility question than anything else to be honest. Our reasoning was that code which uses this dependency will then have to exactly do the same procedure of hashing as the contract does. Mirroring the code into upstream dependencies.

It's really a question about 'who should own how params are hashed' and having this as close to the contract interaction as possible made sense to us.

The contract has the requirement that hashing happens in some exact way so it's the direct owner of that requirement. Not having to worry about that in this or other dependencies would've been nice.

mirceanis · 2022-08-17T12:08:53Z

This is more of a code style and accessibility question than anything else to be honest. Our reasoning was that code which uses this dependency will then have to exactly do the same procedure of hashing as the contract does. Mirroring the code into upstream dependencies.

It's really a question about 'who should own how params are hashed' and having this as close to the contract interaction as possible made sense to us.

I see what you mean.
The way I see this, the "signed" methods require a 2 step process, and both steps could be provided by this library.

Step 1, the DID controller calls some createDoSomethingRequest() method, where they are expected to generate the signature
Step 2, the request + signature is sent to the transaction signer, which calls the actual methods on the contract, or rather a method from EthrDidController (paying the ethereum transaction fee along the way).

Example:

DID owner calls createSetAttributeSignerRequest(identity, attrKey, attrValue, rawSigner), which returns an object like:
{ operation: setAttribute, identity: 0x address of the DID, attrKey: <32 byte string>, attrValue: , signature: MetaSignature }
controller sends this object to the fee payer
fee payer calls setAttributeSigned() on the contract with the above parameters.

Does this make sense? What do you think?

src/helpers.ts

mirceanis · 2022-08-17T11:41:55Z

src/helpers.ts

+  dataBytes: Uint8Array,
+  didReg: Contract
+) {
+  const nonce = await didReg.nonce(signerAddress)


Yes. It's about compatibility.
The older contract, which is deployed to mainnet and ethereum testnets tracks the nonce as a property of the owner, and not as a property of the signer.
Most of the time these are the same address, until the owner of the DID is changed, at which point this becomes an issue.

lleifermann · 2022-08-18T12:30:16Z

@mirceanis

I've just added changes implementing the things we discussed about. For each meta transaction method we now have a correlating createHash function which generates the correct hash with the needed input parameters.
This means that this library really does not have to deal with the key material at all and it fully stays in the callers hands. We just generate the hash and play it back where it then gets signed (for instance in the kms plugin).

Originally we wanted to put this into some abstraction with only one method but that added a lot of complexity. This approach should be rather straight forward to understand and more importantly debug 😄

Not sure exactly on how to test the nonce function against the older version of the contract though. Do you have an example for this with the ganache setup this repo is running with?

mirceanis · 2022-08-18T19:26:35Z

I've just added changes implementing the things we discussed about. For each meta transaction method we now have a correlating createHash function which generates the correct hash with the needed input parameters. This means that this library really does not have to deal with the key material at all and it fully stays in the callers hands. We just generate the hash and play it back where it then gets signed (for instance in the kms plugin).

I love this approach!

Not sure exactly on how to test the nonce function against the older version of the contract though. Do you have an example for this with the ganache setup this repo is running with?

I don't know either, I haven't done anything like this yet so I don't have an example.
The steps to reproduce weird behavior in the old contract:

create a DID
changeOwner
setAttributeSigned (increments nonce for the old owner)
setAttributeSigned again << fails with bad_signature if nonce is loaded from signer and not from the owner.

Even worse bug in the old contract:

changeOwnerSigned (increments nonce for the old owner, but hash is computed with nonce from the signer)
add/removeDelegateSigned (increments nonce for the old owner, but hash is computed with nonce from the signer)
This means that owner and delegates can be changed at most once using metaTX, until the owner and signer nonces diverge.

Perhaps @piouslove can jump in with an example, or maybe @nichonien has run into this issue at EW

lleifermann · 2022-08-19T14:54:16Z

@mirceanis Now everything should be in-place i believe 😄 - For the tests i had to copy the ABI of the old version of the contract as old packages don't export the abi sadly. But the two new tests shoud cover the compatability of nonces.

mirceanis

This looks good.

I think the newer versions of the contract should list a version, so that the user doesn't have to guess if the deployment they are interacting with tracks nonces properly or not. This will also come in handy for further upgrades.
What do you think?

# [6.2.0](6.1.0...6.2.0) (2022-09-05) ### Features * add controller support for meta/signed transactions ([#164](#164)) ([ce93e70](ce93e70))

uport-automation-bot · 2022-09-05T06:36:49Z

🎉 This PR is included in version 6.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

lleifermann added 3 commits August 16, 2022 15:36

add first meta contract interaction and test

b17fdde

add remaining meta transaction functions and tests

2bc24f6

change order of tests to change owner last to not break flow

0be8967

lleifermann commented Aug 17, 2022

View reviewed changes

src/__tests__/resolver.test.ts Show resolved Hide resolved

mirceanis requested changes Aug 17, 2022

View reviewed changes

lleifermann added 2 commits August 17, 2022 12:58

remove ethers dep and replace it with ethersproject counterparts

8dbc9db

add missing imports

2837d81

mirceanis approved these changes Aug 17, 2022

View reviewed changes

lleifermann added 2 commits August 18, 2022 14:18

create dedicated hashing functions for each meta method

7946ea3

remove unused imports

54e3a95

lleifermann requested a review from mirceanis August 18, 2022 12:32

lleifermann added 2 commits August 19, 2022 16:49

add tests and functionality for nonce tracking compat

ac2e48a

rename variable to be scoped to the nonce compat

e134688

mirceanis approved these changes Sep 5, 2022

View reviewed changes

mirceanis merged commit ce93e70 into decentralized-identity:master Sep 5, 2022

uport-automation-bot pushed a commit that referenced this pull request Sep 5, 2022

chore(release): 6.2.0 [skip ci]

2737b6d

# [6.2.0](6.1.0...6.2.0) (2022-09-05) ### Features * add controller support for meta/signed transactions ([#164](#164)) ([ce93e70](ce93e70))

uport-automation-bot added the released label Sep 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for meta/signed transactions #164

Add support for meta/signed transactions #164

lleifermann commented Aug 17, 2022

lleifermann commented Aug 17, 2022

mirceanis left a comment

mirceanis Aug 17, 2022

fermentfan Aug 17, 2022

mirceanis Aug 17, 2022

mirceanis commented Aug 17, 2022

lleifermann commented Aug 17, 2022

mirceanis commented Aug 17, 2022

mirceanis Aug 17, 2022

lleifermann commented Aug 18, 2022

mirceanis commented Aug 18, 2022

lleifermann commented Aug 19, 2022

mirceanis left a comment

uport-automation-bot commented Sep 5, 2022

Add support for meta/signed transactions #164

Add support for meta/signed transactions #164

Conversation

lleifermann commented Aug 17, 2022

lleifermann commented Aug 17, 2022

mirceanis left a comment

Choose a reason for hiding this comment

mirceanis Aug 17, 2022

Choose a reason for hiding this comment

fermentfan Aug 17, 2022

Choose a reason for hiding this comment

mirceanis Aug 17, 2022

Choose a reason for hiding this comment

mirceanis commented Aug 17, 2022

lleifermann commented Aug 17, 2022

mirceanis commented Aug 17, 2022

mirceanis Aug 17, 2022

Choose a reason for hiding this comment

lleifermann commented Aug 18, 2022

mirceanis commented Aug 18, 2022

lleifermann commented Aug 19, 2022

mirceanis left a comment

Choose a reason for hiding this comment

uport-automation-bot commented Sep 5, 2022