veramo icon indicating copy to clipboard operation
veramo copied to clipboard

feat: add support for did:ethr signed/meta transactions

Open lleifermann opened this issue 1 year ago • 3 comments

This PR adds support for sending transactions on behalf of another did:ethr.

This is archived via updates to the underlying dependencies https://github.com/uport-project/ethr-did & https://github.com/decentralized-identity/ethr-did-resolver.

To keep abstraction intact, this behavior may be triggered by passing a metaIdentifier to a didManager action which is picked up by the did-provider-ethr. If a metaIdentifier is present the underlying controller is passed this identifier as a signer which then signs and pays for the transactions.

The methods addKey & addService now detect this presence as well and prepare the transaction differently then. Overall we tried to keep changes to other plugins as minimal as possible.

Some todo'ish items:

  • Right now only addKey and addService have meta capabilities. changeOwner and addDelegate are NOT implemented yet. @mirceanis - Not sure how to tackle this with the current AbstractIdentifierProvider. We would have to put these actions into the updateIdentifier method. But with the current abstraction the did-provider-ethr would receive a didDocument there. Not sure how to proceed here with a good approach. We could change the abstract signature of updateIdentifier to something more easily hand-able (like actions being passed down or something).

  • We had to introduce a new 'algorithm' to the kms-local provider. All other existing options for creating eth signatures either modify the input data, or hash it again. We just need clean digest with the r, s and v params easily accessible afterwards.

  • The tests for the did-provider-ethr changes will fail as i am awaiting an update to ethr-did which fixes an encoding bug we found.

  • After some testing and fiddling around we also noticed that if this package does not pass any options down to did-ethr some defaults for transactions will be chosen, which could result in the transaction failing. Gaslimit and Gasprice are always overridden as defaults. AFAIK if etherjs is not passed anything it will make a best guess for determining these parameters.

lleifermann avatar Oct 12 '22 20:10 lleifermann

@mirceanis i removed the ethers dependency and moved the ethr_rawSign into it's own method and is also now advertised in the meta.algorithm array for Secp256k1 keys.

Regarding the signature flow we wanted to keep it as straight forward as possible. But i see your point in separating these things.

Whats your take on the missing changeOwner and addDelegate functions? Should we place those in the updateIdentifier function? AFAIK this would require a small change in the Abstract IdentifierProvider as it right now passes passes a DidDocument down. We figured a key/value map of possible UpdateActions would fit better as the concrete implementation of the provider can then expose them properly. (did-provider-ethr providing changeOwnerAction and addDelegateAction for example). Other providers then may provide other actions which fit in the updateIdentifier action.

With the current implementation we would need to do some delta comparison i believe (current did doc vs new did doc = wanted change)

lleifermann avatar Oct 13 '22 13:10 lleifermann

@mirceanis i removed the ethers dependency and moved the ethr_rawSign into it's own method and is also now advertised in the meta.algorithm array for Secp256k1 keys.

That's great, thanks!

Regarding the signature flow we wanted to keep it as straight forward as possible. But i see your point in separating these things.

It makes sense. What you did here is very valuable too, even if the full power of meta-tx is not used yet.

Whats your take on the missing changeOwner and addDelegate functions? Should we place those in the updateIdentifier function? AFAIK this would require a small change in the Abstract IdentifierProvider as it right now passes passes a DidDocument down. We figured a key/value map of possible UpdateActions would fit better as the concrete implementation of the provider can then expose them properly. (did-provider-ethr providing changeOwnerAction and addDelegateAction for example). Other providers then may provide other actions which fit in the updateIdentifier action.

With the current implementation we would need to do some delta comparison i believe (current did doc vs new did doc = wanted change)

This is a tricky problem, because a change in AbstractIdentifierProvider would have to make sense for most DID methods (or at least more than the one we're dealing with now). My gut reaction is that custom control logic, that only makes sense to a certain DID method should be built as its own plugin and not try to morph the DIDManager API, but DID controller updates are probably common? We started discussing this issue in #583 (wow, long time ago!), so we can continue the chat about this there.

mirceanis avatar Oct 13 '22 13:10 mirceanis

Only thing missing here is now the failing import of the ethr-did-controller. Once the downstream bundling of ethr-did and ethr-did-controller is fixed i can include the version here. Then everything should be good to go as far as i can tell.

For the addDelegate and changeOwner stuff i would propose a solution in #583

I already gave the ethr-did bundling a crack with @mirceanis changes to ethr-did-controller here -> https://github.com/uport-project/ethr-did/pull/105. Tests are still failing though but i will give this another look today.

lleifermann avatar Oct 14 '22 10:10 lleifermann

Codecov Report

Merging #1031 (8182ac6) into next (125bf42) will increase coverage by 0.55%. The diff coverage is 82.70%.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #1031      +/-   ##
==========================================
+ Coverage   80.25%   80.80%   +0.55%     
==========================================
  Files         118      124       +6     
  Lines        4056     4516     +460     
  Branches      875      966      +91     
==========================================
+ Hits         3255     3649     +394     
- Misses        798      864      +66     
  Partials        3        3              

codecov[bot] avatar Oct 18 '22 13:10 codecov[bot]