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.

Sign up for GitHub

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

Jump to bottom

Batch gateway #1

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Batch gateway #1

wants to merge 15 commits into from

Conversation

makoto
Copy link
Member

@makoto makoto commented Aug 29, 2022 •
edited
Loading

@makoto makoto requested a review from Arachnid August 29, 2022 16:04
@makoto makoto mentioned this pull request Aug 29, 2022
Closed
Arachnid
Arachnid requested changes Aug 29, 2022
View reviewed changes
LICENSE Outdated Show resolved Hide resolved
README.md

```
contract OffchainResolver is IExtendedResolver, ERC165, OffchainMulticallable {
Copy link
Member

Choose a reason for hiding this comment

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

?? What is this supposed to show the user?

README.md
- Encode `resolver(dnsname, addrData)` into `callData`
- Combine `callData` into the array of `callDatas`
- Call `offchainResolver.multicall(callDatas, {ccipReadEnabled:true})` with CCIP-read feature enabled
Copy link
Member

Choose a reason for hiding this comment

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

I suppose you could use it like this, but the intention was that the main use case would be for things like the universal resolver to use the batch gateway, transparently to user code.

src/client.js

const IResolverService = new ethers.utils.Interface(IResolverService_abi);

function getDnsName(name) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you import mafintosh/dns and use name.encode, or at least copy the code here?

src/client.js
name: chainName,
ensAddress,
});
(async () => {
Copy link
Member

Choose a reason for hiding this comment

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

This seems to replicate a lot of the functionality of the universal resolver. Why not just use that?

src/server.ts
})).then((values) => {
// reject non 200 responses
// @ts-ignore
return values.filter(v => v.status === 'fulfilled').map((v:any) => v.value)
Copy link
Member

Choose a reason for hiding this comment

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

What if all the gateways return non-200 responses?

src/server.ts
const sender = request.to;
let responses = await Promise.all(
data.map((d: any) => {
return Promise.allSettled(d.urls.map((url: string) => {
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure we want to query them all in parallel? This could result in serious query amplification / DoS issues.

test/server.test.ts
const host = 'https://example.com'
const host2 = 'https://example2.com'
const response = GatewayI.encodeFunctionResult('query', [[1]])
describe('makeServer', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Can you add an E2E test using Ethers?

@makoto
Copy link
Member Author

makoto commented Aug 31, 2022

@Arachnid as discussed, I tried to move OffchainMulticallable.multicall from OffchainResolver to UniversalResolver but getting SignatureVerifier: Invalid sigature because the batch gateway calls gateway with UniversalResolver address as sender not offchainResolver. This is the flow

### Before

- a.1 `OffchainResolver` implements `OffchainMulticallable.multicall()`
- a.2 client calls `OffchainResolver.multicall(calldatas, {ccipReadEnabled:true})`
- a.3 batch gateway calls `feth(gatewayurl/{offchaiResolver.address}/{callData})`
- a.4 gateway signs response with `offchaiResolver.address` as a signer and returns `response`
- a.5 `OffchainResolver.multicallCallback(response)` calls `OffchainResolver.resolveWithProof(response)`
- a.6 `OffchainResolver.resolveWithProof(response)`  checks that it is signed by `offchaiResolver.address` and returns address

### Now

- b.1 `UniversalResolver` implements `OffchainMulticallable.multicall()`
- b.2 client calls `UniversalResolver.multicall(calldatas, {ccipReadEnabled:true})`
- b.3 batch gateway calls `feth(gatewayurl/{UniversalResolver.address}/{callData})`
- b.4 gateway signs response with `UniversalResolver.address` as a signer and returns `response`
- b.5 `UniversalResolver.multicallCallback(response)` calls `OffchainResolver.resolveWithProof(response)`
- b.6 `OffchainResolver.resolveWithProof(response)`  checks that it is NOT signed by `offchaiResolver.address` and throws `SignatureVerifier: Invalid sigature`

Ideally b.3 should call with feth(gatewayurl/{OffchainResolver.address}/{callData}) not feth(gatewayurl/{UniversalResolver.address}/{callData}) but I don't know how to get the offchain address.

UniversalResolver.callWithOffchainLookupPropagation function encodes the offchain resolver address as part of extraData section here but I don't think client sends extraData to the batch gateway and also the batch gateway shouldn't be specific to the implementation of UniversalResolver

@Arachnid
Copy link
Member

Arachnid commented Sep 1, 2022

Ideally b.3 should call with feth(gatewayurl/{OffchainResolver.address}/{callData}) not feth(gatewayurl/{UniversalResolver.address}/{callData}) but I don't know how to get the offchain address.

You're right, you will need to modify the batch gateway interface to include the address that raised the offchain request in OffchainLookupCallData.

@makoto
Copy link
Member Author

makoto commented Sep 1, 2022

@Arachnid
How about this approach? ensdomains/ens-contracts@53679f6

@Arachnid
Copy link
Member

Arachnid commented Sep 1, 2022

I left a comment, but I don't understand how it would help here?

@makoto
Copy link
Member Author

makoto commented Sep 9, 2022

@Arachnid Can you comment about ensdomains/ens-contracts@53679f6#r82792383 so that you get notification on the conversation

@makoto
Copy link
Member Author

makoto commented Sep 14, 2022

ensdomains/ens-contracts@53679f6#r83841224

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Arachnid Arachnid Arachnid requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@makoto @Arachnid