Skip to content

Conversation

@smk762
Copy link
Contributor

@smk762 smk762 commented Jan 3, 2022

What was wrong?

Noticed this at https://docs.ens.domains/dapp-developer-guide/resolving-names

image

How was it fixed?

  • added get_text method
  • added related ABI entry

Usage: ns.get_text('dragonhound.eth', "avatar")
Returns: https://i.imgur.com/gAD7BxX.jpg

Todo:

Cute Animal Picture

image

Copy link
Collaborator

@kclowes kclowes 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 for this PR! The code looks solid, but it will need a test before we merge. Let me know if you want some direction in that regard!

Edit: The failing integration tests are just flaky, so feel free to ignore timeout errors on those.

@smk762
Copy link
Contributor Author

smk762 commented Jan 4, 2022

I can add tests, no problem. Would this be the correct file to do so? https://github.com/ethereum/web3.py/blob/master/tests/ens/test_ens.py

I noticed in other places in your tests code it was using TESTER.eth but there are no text record entries I can see for that at https://app.ens.domains/name/TESTER.eth/details

Can use an alternative, just concerned that if the text record is changed in future it may result in failed test.

@payton
Copy link

payton commented Jan 21, 2022

@kclowes just bumping this for @smk762 's question about test location / strategy.

I was looking to use / add this functionality and came across the PR :)

@kclowes
Copy link
Collaborator

kclowes commented Jan 21, 2022

Thanks for the bump @payton!

@smk762 sorry, I don't know how I missed this! We get a lot of issues so feel free to bump if you haven't heard in a few days.

Would this be the correct file to do so? https://github.com/ethereum/web3.py/blob/master/tests/ens/test_ens.py

It looks like the public API methods all have their own file in tests/ens, so I would just make a new file and add tests there.

I noticed in other places in your tests code it was using TESTER.eth but there are no text record entries I can see for that at https://app.ens.domains/name/TESTER.eth/details

Can use an alternative, just concerned that if the text record is changed in future it may result in failed test.

I would just use an alternative if there is one, maybe one that seems like it has staying power :) if tests fail, we can re-evaluate.

Thank you!

@kclowes
Copy link
Collaborator

kclowes commented Mar 17, 2022

I started working on tests for this, and realized that we probably need a set_text method as well. I'll PR that separately and then I'll add a quick test here and merge. Thanks @smk762!

@kclowes
Copy link
Collaborator

kclowes commented Mar 18, 2022

Just kidding about the separate PR, they're more inter-related than I thought. I'll just add to this PR.

@kclowes
Copy link
Collaborator

kclowes commented Apr 6, 2022

This just got merged in #2413 and #2395. Thanks for getting this rolling @smk762!

@kclowes kclowes closed this Apr 6, 2022
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