Skip to content

Gluon milestone 2 #169

Merged
semuelle merged 8 commits intow3f:masterfrom
tearust:master
Jun 7, 2021
Merged

Gluon milestone 2 #169
semuelle merged 8 commits intow3f:masterfrom
tearust:master

Conversation

@kevingzhang
Copy link
Copy Markdown
Contributor

Milestone Delivery Checklist

@mmagician
Copy link
Copy Markdown
Contributor

Thanks for another submission! As always, we'll check it as soon as we can.

@alxs alxs changed the title milestone 2 Gluon milestone 2 May 12, 2021
@semuelle semuelle self-assigned this May 12, 2021
@semuelle
Copy link
Copy Markdown
Contributor

Hi Kevin, I am looking into the delivery right now. I am trying to run the unit tests for the webapp, but I'm just getting multiple Jest encountered an unexpected token. Any idea what's missing here?

@liyangwood
Copy link
Copy Markdown
Contributor

oh, we forgot to config jest after separate the share module as ES module. So we will recheck and fix the unit-test sooner. thanks.

@semuelle
Copy link
Copy Markdown
Contributor

Hey guys, removing a past recovery activity results in a StillActive error message on my end. Any idea what's wrong?

@semuelle
Copy link
Copy Markdown
Contributor

Sorry, more questions.

  • What are P2 P3?
  • How is initiating recovery from a mobile phone different? Is this intended for individuals using their second account as social recovery option?
  • On the same topic, what does Recovery pallet doesn't have 2nd account support. mean? How is that an issue here? Second account in the sense of "two accounts being able to recover another"?

@liyangwood
Copy link
Copy Markdown
Contributor

Hi, we update the gluon-app unit-test with tearust/gluon-app@1209c6f.

@liyangwood
Copy link
Copy Markdown
Contributor

Hey guys, removing a past recovery activity results in a StillActive error message on my end. Any idea what's wrong?

Looks like remove the recovery before close it. https://github.com/tearust/gluon-app/blob/milestone-2/readme.md, This is our ideally test process. so kindly to point out which step got that error or the reproduce procedures.

@kevingzhang
Copy link
Copy Markdown
Contributor Author

Sorry, more questions.

  • What are P2 P3?

P2 and P3 are the concepts of Gluon multisig private keys. Along with P1, all 3 keys together generate the real public key via Schnorr public key aggregation. The background information please refer to our original open grant application https://github.com/w3f/Open-Grants-Program/blob/master/applications/Gluon_decentralized_hardware_crypto_wallet_services.md

Here is the diagram in that proposal
diagram

  • How is initiating recovery from a mobile phone different? Is this intended for individuals using their second account as social recovery option?

In Gluon's design, the address of user's web browser extension is one address. The address on user's mobile phone is another address. They are different. Therefore the mobile phone can be used as 2FA device. They are paired (See our milestone1 deliverable). Our Layer1 blockchain will only let go when both addresses sign tx.

Because the out-of-box social recovery pallet doesn't support 2FA, so the 2nd address cannot be added into the recovery pallet without modifying the original code. We would prefer to leave the pallet as is. So we did not design initializing recovery from a mobile phone. Another consideration is that we do not require users to remember the private key in their mobile phone. If the phone lost, there is no way to get that address back. That's the reason we need social recovery , right? To my personal experiences, lost of a phone is more common than lost of a computer.

  • On the same topic, what does Recovery pallet doesn't have 2nd account support. mean? How is that an issue here? Second account in the sense of "two accounts being able to recover another"?

As explained above. That is the limitation of existing social recovery pallet. We try not to change the code.

Let me know if I did not make it clear, I will try to explain it in detail when requested

@semuelle
Copy link
Copy Markdown
Contributor

Hi Kevin, thanks for the response. I had to take a more thorough look at your documentation to understand the difference between using two devices for 2FA and for recovery. Since recovery is only a small part of the grant, I think we can do without mobile recovery.

The rest is looking pretty good. Just one more request: can you point me to where P2, P3 and old account activity are suspended (4.0 and 4.1 in your task list)?

@liyangwood
Copy link
Copy Markdown
Contributor

liyangwood commented May 23, 2021

Hi, @semuelle
We put the suspend feature here. https://github.com/tearust/gluon-pallet/blob/milestone-2/gluon/src/lib.rs#L900-L902

That means when an account has an active recovery process, that account can not transfer any asset to others. an Error named "AssetSuspend" will return to him.

We also update the suspend info with tearust/gluon-app@d489127#diff-5a831ea67cf5cf8703b0de46901ab25bd191f56b320053be9332d9a3b0d01d15
So please get more details from https://github.com/tearust/gluon-app/blob/milestone-2/readme.md#asset-suspend

@semuelle
Copy link
Copy Markdown
Contributor

Hi @liyangwood, thanks for the clarification. I see how the links you provided suspend old account activity. How about Task 4.0, Suspend old P2 P3? Where and when does this happen?

Feel free to update your delivery document to include links to the code specific to each deliverable. That makes verification of the deliverable much easier for us.

Add more description for task 4.1
@liyangwood
Copy link
Copy Markdown
Contributor

Hi @semuelle Thanks for checking.
We had updated the delivery document.

Copy link
Copy Markdown
Contributor

@liyangwood liyangwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update description for task 4.1

@semuelle
Copy link
Copy Markdown
Contributor

Thanks for that, @liyangwood. Could you also provide a link to the matching code for Task 4.0, please?

@kevingzhang
Copy link
Copy Markdown
Contributor Author

Thanks for that, @liyangwood. Could you also provide a link to the matching code for Task 4.0, please?

Hi @semuelle,
Thank you for reviewing our commits.

As I explained in previous comments (#169 (comment)), the P2, and P3 suspending in layer1 only. So the task 4.0 is no longer valid. Only 4.1 should be existing. I can remove task 4.0 if you do not mind.

@semuelle
Copy link
Copy Markdown
Contributor

I did not read that from your comment. The link you provided under the task seems misleading to me, then.

Can you please go through the list of deliverables and tasks defined in your grant contract and your deliverable document, clearly mark which have been implemented and where, and which have not? The committee will then decide whether a contract amendment is necessary.

@kevingzhang
Copy link
Copy Markdown
Contributor Author

Hi @liyangwood, thanks for the clarification. I see how the links you provided suspend old account activity. How about Task 4.0, Suspend old P2 P3? Where and when does this happen?

Feel free to update your delivery document to include links to the code specific to each deliverable. That makes verification of the deliverable much easier for us.

Let me give you an explanation on P2, P3 and how to suspend P2 P3, and what is task 4.0, 4.1. How they are related.

Based on our previous conversation, I assume you have know that

  • We do not have mobile account in recovery. This is become the existing social recovery pallet doesn't support 2nd account.
  • Two of [P1, P2, P3] working together can sign the transaction.
  • If user starts social recovering process, we need to suspend the account so that no one can take asset away.

Ok, now let's review what suspending means. Based on @liyangwood explanation,

We put the suspend feature here. https://github.com/tearust/gluon-pallet/blob/milestone-2/gluon/src/lib.rs#L900-L902
These two lines of code disabled any transaction to this account. This is where task 4.0 4.1 implemented.

In our original design, we believe there are two places we need to implement the suspend action:

  • In task 4.0, disable from the wasm actors, we call it Tea Leaf.
  • In task 4.1, disable from the blockchain, we call it layer 1.
    But because social recovery doesn't support 2nd account, there is no need to disable from the mobile. That means there is no need to disable it from wasm actors, that is Tea Leaf. That also means that Task 4.0 can be done perfectly by Task 4.1. Because Task 4.1 can disable/suspend the account in blockchain ONLY.

That's why I was saying Task 4.0 is not necessary. Since you are asking where we implemented it, I can point out the same code location as Task 4.1. But this location is in blockchain only, not in Tea Leaf. Again, the suspending action can be done by Task 4.1 along because social recovery pallet doesn't support 2nd account.

Just in case you do not know why the 2nd account related to wasm actor (Tea Leaf). Here is the answer. The 2FA from mobile is handled by the wasm actor. If there is no 2FA, no action in wasm actor is required.

You can schedule a meeting with me, I can explain it to you over zoom. how about that?

@kevingzhang
Copy link
Copy Markdown
Contributor Author

Hi @semuelle ,
Just in case if you still doubt what relationship between P2 P3 and 2nd account or 2FA, here is the explanation.
P1 is not controlled by the layer 2. Layer 2 is the wasm actor code running inside the TEA Nodes. Layer 2 is NOT blockchain.
P2 and P3 are controlled by the layer 2. The layer 2's wasm code can decide if the 2FA verified successfully then let go continue sign.
Controlling P2 and P3 is useful when users want to sign a regular transaction. But the social recovery is not a regular transaction. User has lost his phone and computer, there is no way for him to use his original mobile phone's 2nd account to do 2FA. Not to mention social recovery pallet doesn't support 2nd account feature.
The goal of suspending P2 and P3 is to protect user when someone steal his phone and computer, trying to impersonate him. Or, on the other hands, the user did not lose his phone and computer, but someone trying to impersonate him to run a social recovery. To prevent this happen, we suspend the P2 P3 so that the user will know someone is trying to impersonate him, and no assets lost in this case.

Now let's get back to our implementation. There is no 2nd account (2FA, or mobile account) support in Social Recovery Pallet. Task 4.0 can be done automatically by Task 4.1. Because as long as we disable/suspend the transaction (you can see the two lines of code), all transactions are cancelled

      let flag = Self::can_transfer_asset(&sender, &to);            
      debug::info!("----- : {:?}", flag); 
      ensure!(flag == true, Error::<T>::AssetSuspend);

That's why I said, Task 4.0 and Task 4.1 have the same implementation code.

The reason why we had Task 4.0 in the original application is that we did not Social Recovery Pallet doesn't support 2nd account at that moment. Right now, we know 1. it doesn't support 2. it is no necessary since the phone and computer are lost, even it supports, it cannot be used.

I understood it is complicated and hard to understand.

@semuelle
Copy link
Copy Markdown
Contributor

Thanks for the explanations, Kevin. To clarify my issue: I don't see a suspension of keys in the three lines of code you point to. They simply check a condition and cause the function to fail if it is not satisfied. It is perfectly fine if a task is fulfilled without any extra steps, I just don't see either satisified in the code you linked to or the function it references.

Do you have time to go over the delivery with @Lederstrumpf and me on Monday or Tuesday around 16:00 UTC?

@kevingzhang
Copy link
Copy Markdown
Contributor Author

Thanks for the explanations, Kevin. To clarify my issue: I don't see a suspension of keys in the three lines of code you point to. They simply check a condition and cause the function to fail if it is not satisfied. It is perfectly fine if a task is fulfilled without any extra steps, I just don't see either satisified in the code you linked to or the function it references.

Do you have time to go over the delivery with @Lederstrumpf and me on Monday or Tuesday around 16:00 UTC?

That's exactly the design goal that P2 and P3 no longer valid during the social recovery period. Once the social recovery is completed or canceled by the user, the check will succeed and continue.

here is my calendar calendly.com/kevinzhang, you can find the best time fits your time zone.

@semuelle
Copy link
Copy Markdown
Contributor

semuelle commented Jun 3, 2021

Just for documentation purposes, the grant agreement was altered to move the key suspension during social recovery to M3.

@semuelle
Copy link
Copy Markdown
Contributor

semuelle commented Jun 4, 2021

Hey Kevin, since there is no separate deliverable covering inline documentation in your milestones, could you add some inline documentation to gluon-pallet?

@kevingzhang
Copy link
Copy Markdown
Contributor Author

Of course I'd love to. but may I add the comments in our M3 given we will need to mgirate to the latest version of substrate and major code changes. All inline comments I made in current version may ned to be redone.

@semuelle
Copy link
Copy Markdown
Contributor

semuelle commented Jun 5, 2021

Okay, I'll add a note to M3.

@semuelle
Copy link
Copy Markdown
Contributor

semuelle commented Jun 7, 2021

Hey Kevin,
this milestone is hereby accepted. 🎉

Feel free to keep us in the loop about any possible changes in milestones or deliverables, whether they are delays or just rearrangements. That often makes the submission and evaluation much smoother later.

I will forward your invoice for processing.

@semuelle semuelle merged commit 66e1e38 into w3f:master Jun 7, 2021
@semuelle
Copy link
Copy Markdown
Contributor

semuelle commented Jun 7, 2021

@kevingzhang, there seems to be a mismatch between the payment address in the contract and the one in the invoice. Could you either send a new invoice or submit a PR to change the contract?

@kevingzhang
Copy link
Copy Markdown
Contributor Author

kevingzhang commented Jun 7, 2021 via email

@kevingzhang
Copy link
Copy Markdown
Contributor Author

kevingzhang commented Jun 7, 2021 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants