feat: add signOnly flag and logic for add/remove key/service

[radleylewis]

What issue is this PR fixing

Closes #1373

What is being changed

Due to constraints within the secure MetaMask Snap context, it is not possible to invoke the didManagerAdd(Remove)Service or didManagerAdd(Remove)Key methods on the did-manager successfully. Refer here for the list of blocked RPC methods.

As outlined in the referenced issue, the changes proposed herein allow for the transaction to be signed only, through the addition of a new signOnly flag within one agent (in the demonstration case, by an agent in the constrained MetaMask Snap environment), and then submitted from another context (for example via the ethers.InfuraProvider in another context).

Quality

Check all that apply:

• I want these changes to be integrated
• I successfully ran pnpm i, pnpm build, pnpm test, pnpm test:browser locally.
• I allow my PR to be updated by the reviewers (to speed up the review process).
• I added unit tests.
• I added integration tests.
• I did not add automated tests because I first want to have the code reviewed by the maintainers, to get their feedback on the approach, and am happy to add tests as part of the final PR.

Details

I have successfully tested this (with the did-ethr-provider on the sepolia testnet).

[mirceanis]

This looks much better than before!

I think we are missing a couple of things before we can merge this:

1. tests to validate the new behavior
2. (optional, but highly recommended) even more simplification of the code:

We now have 3 paths for adding/removing an attribute:

• a. sign and send a transaction directly. (default case)
• b. sign a message using one key and sign and/or send a transaction using another key (metaIdentifierKeyId)
• c. sign a message using one key (signOnly param)

Ideally, we would use the functionality from c. as a first step in b.

[radleylewis]

This looks much better than before!

I think we are missing a couple of things before we can merge this:

1. tests to validate the new behavior
2. (optional, but highly recommended) even more simplification of the code:

We now have 3 paths for adding/removing an attribute:

• a. sign and send a transaction directly. (default case)
• b. sign a message using one key and sign and/or send a transaction using another key (metaIdentifierKeyId)
• c. sign a message using one key (signOnly param)

Ideally, we would use the functionality from c. as a first step in b.

Hi @mirceanis. I have pushed some tests up which cover the introduced signOnly behaviour. Just a few points to clarify:

• The tests require an INFURA_API_KEY because the provider needs to be instantiated with a network RPC url. I noticed there are some existing tests which have been commented out, and which include an InfuraApiKey variable (perhaps these tests have been disabled for the same reason?)
• I can write additional tests, noting there presently are none, but want to check with you first about secrets/env variables, as these could be setup at the repo level, or you may already have some approach.
• Overall add/remove key/service have tests applied and these can be expanded upon, perhaps in a new PR, just let me know your thoughts.
• I agree that the code can be simplified, there is one caveat however, as you see that the final else block does not require a sigature (eth_sendTransaction as opposed to eth_sendRawTransaction). This means that the logic is not as straight forward as simply returning out if the signOnly flag is present. That being said, if you are ok with a conditional block within the else statement, I am ok to proceed. Actually, on a side note, this is where I picked up on the behaviour of the ethr-did-resolver because where no signature is provided it works at present, it is only where a signature is provided that it fails (pertaining to the other PR).

[mirceanis]

This looks much better!
I suggested a key to safely include and after that, I think we can merge this

[mirceanis]

// this key is safe to hardcode as it's limited to some did:ethr contract addresses

Suggested change

	const INFURA_API_KEY = ''
	const INFURA_API_KEY = '3586660d179141e3801c3895de1c2eba'

[radleylewis]

Thanks! I have added the key.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.97%. Comparing base (1c6627c) to head (4db3a2d).
Report is 4 commits behind head on next.

Additional details and impacted files

@@            Coverage Diff             @@
##             next    #1389      +/-   ##
==========================================
+ Coverage   89.95%   89.97%   +0.01%     
==========================================
  Files         176      176              
  Lines       27695    27730      +35     
  Branches     2201     2204       +3     
==========================================
+ Hits        24912    24949      +37     
+ Misses       2783     2781       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mirceanis]

looks good!