veramo
veramo copied to clipboard
feat: add support for did:ethr signed/meta transactions
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
andaddDelegate
are NOT implemented yet. @mirceanis - Not sure how to tackle this with the current AbstractIdentifierProvider. We would have to put these actions into theupdateIdentifier
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 ofupdateIdentifier
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.
@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)
@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
andaddDelegate
functions? Should we place those in theupdateIdentifier
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 possibleUpdateActions
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 theupdateIdentifier
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.
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.
Codecov Report
Merging #1031 (8182ac6) into next (125bf42) will increase coverage by
0.55%
. The diff coverage is82.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