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

Ethereum Name Service #562

Merged
merged 16 commits into from
Nov 25, 2021
Merged

Ethereum Name Service #562

merged 16 commits into from
Nov 25, 2021

Conversation

SionoiS
Copy link
Contributor

@SionoiS SionoiS commented Oct 26, 2021

Please advise on error handling and test cases.

Copy link
Owner

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

Thank you so much for the contribution.

I don't know details of ENS, but I trust it's implemented correctly. Regarding the tests it should be possible to verify payload sent to eth_call.

It would be good to polish docs as well.

I think the error handling is okay, unless there are cases where specific reversion reasons from EVM are useful for the end users.

src/api/eth_ens.rs Outdated Show resolved Hide resolved
src/api/eth_ens.rs Outdated Show resolved Hide resolved
src/api/eth_ens.rs Outdated Show resolved Hide resolved
src/api/eth_ens.rs Outdated Show resolved Hide resolved
src/api/eth_ens.rs Outdated Show resolved Hide resolved
src/api/eth_ens.rs Outdated Show resolved Hide resolved
src/api/eth_ens.rs Outdated Show resolved Hide resolved
src/api/eth_ens.rs Outdated Show resolved Hide resolved
src/api/eth_ens.rs Outdated Show resolved Hide resolved
src/contract/res/ENSRegistry.json Outdated Show resolved Hide resolved
@SionoiS
Copy link
Contributor Author

SionoiS commented Nov 1, 2021

I changed the organisation and added missing functions even those without info and added links to contracts.

Let me know what you think!

Tests, example and docs will have to wait a bit. I received a grant but work start next year.

Copy link
Owner

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

Thank you so much for all the effort and apologies for the delay. Would you mind to fix all the remaining documentation grumbles?

src/contract/ens/public_resolver.rs Outdated Show resolved Hide resolved
src/contract/ens/public_resolver.rs Outdated Show resolved Hide resolved
src/contract/ens/registry.rs Outdated Show resolved Hide resolved
src/contract/ens/reverse_resolver.rs Outdated Show resolved Hide resolved
src/contract/ens/eth_ens.rs Outdated Show resolved Hide resolved
src/contract/ens/eth_ens.rs Show resolved Hide resolved
src/contract/ens/eth_ens.rs Outdated Show resolved Hide resolved
src/contract/ens/public_resolver.rs Outdated Show resolved Hide resolved
src/contract/ens/public_resolver.rs Show resolved Hide resolved
src/contract/ens/reverse_resolver.rs Show resolved Hide resolved
@SionoiS
Copy link
Contributor Author

SionoiS commented Nov 12, 2021

A per ens/mod.rs the only type accessible is ENS.

@tomusdrw most of the documentation your asking for is not user facing. Are you sure? More docs is better I get it.

@tomusdrw
Copy link
Owner

@tomusdrw most of the documentation your asking for is not user facing. Are you sure? More docs is better I get it.

Correct, internal documentation is useful as well and I think we should put a bit of attention and effort there. Also from my experience it happens rather frequently that things that initially were not public end up being used at some point, so we can already be prepared for this :)

@tomusdrw tomusdrw enabled auto-merge (squash) November 25, 2021 21:02
@tomusdrw tomusdrw merged commit 5f8b29e into tomusdrw:master Nov 25, 2021
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.

2 participants