Add ENS resolve support #150
Conversation
Codecov Report
@@ Coverage Diff @@
## main #150 +/- ##
========================================
Coverage 99.74% 99.75%
========================================
Files 67 70 +3
Lines 3899 4028 +129
========================================
+ Hits 3889 4018 +129
Misses 10 10
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
|
||
| describe Ens::Resolver do | ||
|
|
||
| let(:geth_dev_http_path) { "http://127.0.0.1:8545" } |
There was a problem hiding this comment.
We shouldn't be making any http calls from tests. Look into vcr or webmock or simply putting a file with the expected response from the http endpoint in the spec/ directory somewhere.
| let(:geth_dev_http_path) { "http://127.0.0.1:8545" } | |
| let(:geth_dev_http_path) { "http://127.0.0.1:8545" } |
There was a problem hiding this comment.
I've used both vcr and webmock, but given that we live in a time where we can easily run services in test environments, I tend to disagree. GHA can easily run a geth dev node, so... why not? If the container is updated to the latest version, the tests are now running against that (integration), which I think is a good thing. Also, I was just imitating behavior I saw in the existing test suite in an attempt to not stray too far off the beaten path.
There was a problem hiding this comment.
Yes, I needed a way to make sure that the HTTP client works, so I spawned a Geth node within the GHA to prevent relying on third parties. It's fine to reuse this in other test cases IMHO.
| # @param ens_name [String] The ENS name, eg: fancy.eth | ||
| def initialize(client, address = DEFAULT_ADDRESS) | ||
| @client = client | ||
| @contract = Eth::Contract.from_abi( |
There was a problem hiding this comment.
Few things here:
- I would set only the contract filename here, so your object instantiation will not ever fail. Also set the ENS address as an instance variable. You may want to add a
valid?predicate method, but not necessary.
| @contract = Eth::Contract.from_abi( | |
| @address = address | |
| @contract_filename = "ens.json" |
- I would define a constant that points to the ABIs root directory.
ABI_DIRECTORY = File.join(File.dirname(__FILE__), '../../../abis'))If you see this constant being reused by other class (I do), then probably add it to Eth::Constants instead.
- Add a
def parsed_abimethod that reads and parses the JSON file with error handling.
def parsed_abi
JSON.parse(File.read(File.join(ABI_DIRECTORY, @contract_address)
rescue StandardError => e
# log something here
puts "oh no, stuff blew up"
@valid = false
end- Add a memoized
contractmethod that instantiates the `Eth::Contract with the parsed ABI and address.
def contract
@contract ||= Eth::Contract.from_abi(name: 'ENS', address: address, abi: parsed_abi)
end- Use
attr_reader :address, :client, :parsed_abiinstead of accessing instance variables directly.
Co-authored-by: Matt Culpepper <mculp@users.noreply.github.com>
Yeah, I had to fight myself with the encoding piece. Perfect is the enemy of good. |
|
I'll adjust some things based on feedback @mculp tomorrow. |
|
@q9f I was able to make CI happy, two of the tests I had to mark as pending due to: 1) Lack of a library to handle the full extent of text processing required for ENS, and 2) No existing way to test live chains, or to do a full deploy of ENS contracts locally, write to them, etc. If there is an infura/alchemy key in CI secrets, then resolving a known ENS address on mainnet would work... with the small caveat that when that changes the test will need to be updated. |
|
Thank you. I can add an infura key to the pipelines in future. |
This does work to resolve common ENS names to a given address (I'm using it), with the caveat that the name normalization isn't to spec... so it will not work with some edge case names.
Opening this PR per request.