Skip to content
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

Add README to DNS example #1542

Closed
wants to merge 3 commits into from

Conversation

Irene-123
Copy link

Hello there, I'm a blockchain enthusiast and contributing to open-source projects lately. I've been using substrate lately for my college projects and found that this great DNS smart contract doesn't have a README, so I've added it. Thank you!

Copy link
Contributor

@HCastano HCastano left a comment

Choose a reason for hiding this comment

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

Thanks for you PR, but it needs some fixes.

Also, can you wrap everything to ~90 characters?

@@ -0,0 +1,33 @@
# DNS Smart contract

The DNS smart contract is our showcase for executing other smart contracts on-chain.
Copy link
Contributor

Choose a reason for hiding this comment

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

This contract doesn't execute any other contracts, it's self contained

Copy link
Author

Choose a reason for hiding this comment

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

Sure, changed it




> This is a port from the blog post's ink! 1.0 based version of the contract to ink! 2.0.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not relevant, and is also outdated. Our current stable release is ink! 3.0, and the contract compiles with the ink! 4.0-beta.

Copy link
Author

Choose a reason for hiding this comment

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

Okay, I've mentioned it in the readme. Also, I will try compiling it against 4.0 beta

Comment on lines 24 to 25
### Other functionalities to be implemented:
- Transfer bidding
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think anybody is going to update this example tbh

Copy link
Author

Choose a reason for hiding this comment

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

Sure, I'll remove it then for now

Comment on lines 27 to 28
> __Note:__<br/>
> Depending on your Substrate version you might encounter [a ink_lang dependency error](https://github.com/paritytech/ink/issues/506) which is yet to address.
Copy link
Contributor

Choose a reason for hiding this comment

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

Where did you get this? This issue has been closed for 2+ years

Copy link
Author

Choose a reason for hiding this comment

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

I came across this when compiling the contract. While compiling it gives an error of 'ink_lang'
image

But as mentioned above I was compiling it using the 3.0 version.

The main function of this contract is domain name resolution which refers to the retrieval of numeric values corresponding to readable and easily memorable names such as "polka.dot" which can be used to facilitate transfers, voting and DApp-related operations instead of resorting to long IP addresses that are hard to remember.
<br>

![Image](dns_diagram.png)
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the diagram. Can you make a examples/dns/.images folder and put it in there instead?

[this blog post on medium](https://medium.com/@chainx_org/secure-and-decentralized-polkadot-domain-name-system-e06c35c2a48d).

### Description
The main function of this contract is domain name resolution which refers to the retrieval of numeric values corresponding to readable and easily memorable names such as "polka.dot" which can be used to facilitate transfers, voting and DApp-related operations instead of resorting to long IP addresses that are hard to remember.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you update this to maybe show an example of how this translation would work?

Copy link
Author

Choose a reason for hiding this comment

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

This exact paragraph was written in the comments itself of dns.sol and I've grabbed it from the file. But I think we can make it more clear and show what are the parameters this contract uses.

examples/dns/README.md Outdated Show resolved Hide resolved
@HCastano HCastano changed the title UPDATE DNS README.md Add README to DNS example Dec 16, 2022
@codecov-commenter
Copy link

codecov-commenter commented Dec 16, 2022

Codecov Report

Merging #1542 (eec4171) into master (2cd3a0c) will decrease coverage by 6.27%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1542      +/-   ##
==========================================
- Coverage   71.62%   65.36%   -6.27%     
==========================================
  Files         205      205              
  Lines        6295     6288       -7     
==========================================
- Hits         4509     4110     -399     
- Misses       1786     2178     +392     
Impacted Files Coverage Δ
crates/ink/ir/src/ir/contract.rs 0.00% <0.00%> (-100.00%) ⬇️
crates/ink/ir/src/ir/ink_test.rs 0.00% <0.00%> (-87.50%) ⬇️
crates/ink/ir/src/ir/trait_def/mod.rs 0.00% <0.00%> (-81.82%) ⬇️
crates/ink/ir/src/ir/storage_item/mod.rs 0.00% <0.00%> (-73.92%) ⬇️
crates/ink/ir/src/ir/storage_item/config.rs 0.00% <0.00%> (-68.75%) ⬇️
crates/ink/ir/src/ir/blake2.rs 20.83% <0.00%> (-58.34%) ⬇️
crates/ink/ir/src/ir/trait_def/config.rs 7.14% <0.00%> (-57.15%) ⬇️
crates/ink/ir/src/ir/trait_def/item/trait_item.rs 45.90% <0.00%> (-46.04%) ⬇️
crates/ink/ir/src/ir/config.rs 64.00% <0.00%> (-36.00%) ⬇️
crates/ink/ir/src/ir/utils.rs 57.40% <0.00%> (-35.19%) ⬇️
... and 19 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Irene-123
Copy link
Author

Hii @HCastano I have made some changes, please review it and let me know! Thank you

@HCastano
Copy link
Contributor

Hey @Irene-123 so taking another looks it seems like the info in the README is already mostly covered by the doc comments in the code itself. I'm gonna close it for that reason. Thanks for the effort though!

@HCastano HCastano closed this Feb 23, 2023
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.

3 participants