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

Signer :: Implement VET domains #839

Closed
rodolfopietro97 opened this issue May 2, 2024 · 7 comments · Fixed by #867
Closed

Signer :: Implement VET domains #839

rodolfopietro97 opened this issue May 2, 2024 · 7 comments · Fixed by #867
Assignees

Comments

@rodolfopietro97
Copy link
Contributor

rodolfopietro97 commented May 2, 2024

Implement resolveName function into signer and add support of end in populate call.

This will. allow transaction like this:

signer.sendTransaction({ ... to: 'example.vet' })

@victhorbi victhorbi changed the title Signer :: Implement ENS Signer :: Implement VET domains May 2, 2024
@ifavo
Copy link
Contributor

ifavo commented May 13, 2024

@rodolfopietro97 I would be happy to help with the implementation.

I would:

  1. Add a helper utility that allows any input to always return an address would be very helpful (something like
    ensureAddress(nameOrAddress) returns (address) ) and can be made a public export to allow everyone to ensure inputs are always resolved to addresses.
  2. Extend the network constants with Registry address, maybe the resolve util as well, because the interaction has a better performance due less network requests for a lookup (one instead of two).
  3. Because lookups are async, I believe the best place to implement it is within buildTransactionBody to use ensureAddress for all clauses to properties.
  4. As foundation for all the above, a mock deployment on the solo node in the tests would be required. I would check with the ethers test suite and adapt theirs or add a simple custom deployment.

Would any of this be of any help?

Btw.: Adding support in the accounts module would be the logical next step, making sure the passed account is either an address or name.

@rodolfopietro97
Copy link
Contributor Author

Hi @ifavo yes good. You can add the resolution part into accounts module if you want and then call it into signer.
Feel free to ask me if anything

@ifavo
Copy link
Contributor

ifavo commented May 13, 2024

@rodolfopietro97 what branch do you suggest to fork from?

@fabiorigam
Copy link
Member

@ifavo I suggest main

@ifavo ifavo mentioned this issue May 14, 2024
19 tasks
@ifavo
Copy link
Contributor

ifavo commented May 15, 2024

I have started a PR with this:

  • I have introduced a new vnsUtils that currently supports:
    • vnsUtils.resolveName(thorClient, name)
    • vnsUtils.resolveNames(thorClient, [names])
    • vnsUtils.lookupAddress(thorClient, address)
    • vnsUtils.lookupAddresses(thorClient, [names])
  • The VechainBaseSigner relies on it when using resolveName(name) and lookupAddress(address)
  • The transaction module has now a new method resolveNamesInClauses which is called in simulateTransaction and buildTransactionBody to lookup all names with vnsUtils in a single request.
  • vnsUtils.resolveName(name) is also available as shortcut to allow developers to convert names during clause building into addresses, for example when using addresses in contract calls (not the to part)
  • VechainBaseSigner.lookupAddress(address) has been added to support the other direction of address to name lookup too, with identical function header as defined in ethers

I have some trouble testing contract calls, I also don't see a similar test in the project, is there an alternative way to verify contract calls without executing the request? See this example:

    describe('resolveNames([name])', () => {
        test('Should use the correct resolveUtils based on the passed thor client', async () => {
            const name = 'test-sdk.vet';
            jest.spyOn(thorClient.contracts, 'executeCall');
            await vnsUtils.resolveName(thorClient, name);
            expect(thorClient.contracts.executeCall).toHaveBeenCalledWith(
                '0xc403b8EA53F707d7d4de095f0A20bC491Cf2bc94',
                'function getAddresses(string[] names) returns (address[] addresses)',
                [name]
            );
        });
    });

@ifavo
Copy link
Contributor

ifavo commented May 15, 2024

After a quick chat with Rodolfo we decided to mirror ethers interfaces on Signer & Provider for the new resolveName & lookupAddress function:

  • Remove lookupAddress from Signer
  • Add lookupAddress to the Provider
  • Add resolveName also to the Provider
  • Keep resolveName in Signer

@ifavo
Copy link
Contributor

ifavo commented May 15, 2024

@rodolfopietro97 I've made all changes and deployed a name service into the solo node to test vet.domains locally.

I've cleaned up a few things and I believe the PR is ready, if you accept that contract interaction in detail is not tested (see comment above). The integration is still covered and secured by testing the results of the solo node interaction.

To update your local solo container with the name service you'll need to run docker compose build to re-built your local container with the new chaindata.

Would you approve the PR with the current changes? >> #867

rodolfopietro97 added a commit that referenced this issue May 17, 2024
* feat: init name lookup prototype using vet.domains

* refactor: improve name resolving

* test: improve test process

* feat: add support for vet.domains names in clauses

* refactor: improve name lookup on transaction clauses

* refactor: simplify resolving process

* feat: add support for vet.domains primary name lookups

* refactor: improve ethers interface compatibility on provider & signer

* refactor: remove duplicate definition

* test: add vet.domains vns seeding to thor solo-node

* test: improve testing process for vet.domains

---------

Co-authored-by: Rodolfo Pietro Calabrò <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants