-
Notifications
You must be signed in to change notification settings - Fork 383
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
"sig-verify-tutorial" for Round 2 #286
"sig-verify-tutorial" for Round 2 #286
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are there 5,000 files changed in this diff? Can you confirm that you have the latest from master
merged into this PR before I review it? Thanks
Apparently, you included the whole project with the tutorial. There's no need for that, you can only include the source files. My guess is (not a js dev myself) that for example the whole node_modules directory can be omitted and recreated locally when someone pulls the project. |
I have made the required changes and removed all the unnecessary files including the node modules. Kindly review the PR. If you need to review any of the files that I removed, the original repo is here: https://github.com/red-dev-inc/sig-verify-tutorial |
Hello, we're ready to make any requested changes. Aratrika is working on IST and I'm on EDT just fyi. We overlap each morning between 8 AM and 10:30 AM EDT. |
Renamed png for standards compliance per jpop32's comment on our other tutorial's pull request. |
Hello! Are revisions due today? If so, we think we've made the ones needed, but please confirm. If you would like to request more changes, we would be happy to make additional edits, but we will need a day or two more to complete them due to time zone differences, etc. Thanks. We've really enjoyed creating material for this contest. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work. Minor changes requested.
@@ -0,0 +1,342 @@ | |||
# Tutorial: Avalanche Signature Verification in a dApp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
Metamask needs to be installed on your browser, and you need to be connected to the Avalanche Fuji test network (for this tutorial). You can add a few lines of codes to check if your browser has Metamask installed, and if installed, then to which network you are connected. For instance: | ||
|
||
```javascript |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please port from javascript -> typescript
First, you will need to hash the original message. Here is the standard way of hashing the message based on the Bitcoin Script format and Ethereum format: | ||
|
||
|
||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the data type of this code?
|
||
```javascript | ||
function hashMessage() { | ||
let mBuf = Buffer.from(message, 'utf8') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add type to each variable declaration after porting to typescript.
We have ported Bech32 to Solidity from the [Bech32 Javascript library](https://github.com/bitcoinjs/bech32). | ||
There are four functions, **polymod**, **prefixChk**, **encode** and **convert**, used to convert to Bech32 address. | ||
|
||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Data type of code block?
|
||
However, this project uses the **secp256r1** curve. As Avalanche uses **secp256k1** curve, we need to modify the constant values based on this curve. (**Note:** Since modifying cryptographic functions is risky, these are the only modifications we have made.) The constants now look like this: | ||
|
||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Data type of this block of code?
I have made all the required changes to the best of my understanding. Please review the PR and kindly let us know if any more revisions are required. |
We are so honored to have won and to have had this chance to contribute to Avalanche! Thank you so much for the opportunity. The team at red·dev is looking forward to much more to come as the ecosystem grows, and we are 100% committed to its success. Please let us know how we can help going forward. |
One follow-up question. Regarding the signature verification process we describe in this tutorial, we believe it is correct. However, we are not cryptographers. We are just software devs who have some experience with applied cryptography and love building on Avalanche. I'm wondering what level of auditing for correctness our tutorial received? Since a defect in it could allow an attacker to drain large amounts of AVAX from our dapp, it's a concern, especially now since others might follow this tutorial and use the same technique for their dapps. On a related note, we also did a pull request for the ECDSA repo we used (and modified slightly). tdrerup/elliptic-curve-solidity#3 As you can see, they have not had time to look at it yet and appear to be busy with other things. |
This pull request is for red·dev's entry "sig-verify-tutorial" for Round 2 of the Avalanche Tutorials contest. This entry was submitted originally under the name of red·dev's CEO, Christopher David Sturgeon ("Kit"). It is a group effort by part of the red·dev team. Today, team member Aratrika Mukherjee is submitting the request. We look forward to your critique and comments! Thank you.