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

Implement RequesterAuthorizerWithErc721 support #1632

Closed
Tracked by #1610
bbenligiray opened this issue Feb 1, 2023 · 8 comments · Fixed by #1670
Closed
Tracked by #1610

Implement RequesterAuthorizerWithErc721 support #1632

bbenligiray opened this issue Feb 1, 2023 · 8 comments · Fixed by #1670
Assignees
Milestone

Comments

@bbenligiray
Copy link
Member

RequesterAuthorizerWithErc721 is a new authorizer type with the following interface

function isAuthorized(
        address airnode,
        uint256 chainId,
        address requester,
        address token
    ) external view returns (bool)

airnode is the address of the Airnode calling it, requester is the requester contract address, chainId is the chain ID that the requester contract is deployed on (and the respective RRP request is detected on), token is the address of the ERC721 token that is specified in the config of the Airnode.
RequesterAuthorizerWithErc721 will typically be used as a cross-chain authorizer, though this doesn't affect the implementation of the Airnode-side.
RequesterAuthorizerWithErc721 will inherit SelfMulticall (so tryMulticall() will be used for batch static calls). checkAuthorizationStatuses() can't be used for batch authorization checks because that expects a specific authorizer interface that RequesterAuthorizerWithErc721 doesn't support.
RequesterAuthorizerWithErc721 is currently under development under the ChainAPI Github organization but will be audited and made open source soon.
This blocks https://forum.api3.org/t/secondary-proposal-33-chainapi-july-2022-march-2023/1502 so it should be added to a milestone that is not too far away @aTeoke

@aTeoke
Copy link

aTeoke commented Feb 2, 2023

Could you please elaborate what needs be completed on Airnode side to consider this task as done @bbenligiray? It is a bit confusing to me because of "this does not affect implementation on the Airnode-side.". Thanks

@bbenligiray
Copy link
Member Author

this does not affect implementation on the Airnode-side

This was specifically about "RequesterAuthorizerWithErc721 will typically be used as a cross-chain authorizer".

So here's the example config file of an Airnode that uses RequesterAuthorizerWithErc721 (in addition to RequesterAuthorizer)

{
  ...
  "chains": [
    {
      "authorizers": {
        "requesterEndpointAuthorizers": [
          "0xf18c105D0375E80980e4EED829a4A68A539E6178",
          "0xCE5e...1abc"
        ],
        "requesterAuthorizersWithErc721": [
          {
            "address": "0xCE5e...1abc",
            "erc721Address": "0xCE5e...1abc"
          }
        ]
      },
      ...
    },
    ...
  ],
  ...
}

When an Airnode detects requesterAuthorizersWithErc721 being defined for a request, it calls the isAuthorized() function of this for the respective request, and uses erc721Address as the token argument.

@bbenligiray
Copy link
Member Author

bbenligiray commented Feb 8, 2023

RequesterAuthorizerWithErc721 implemented here (pending audit, though that's not a blocker)

@dcroote
Copy link
Contributor

dcroote commented Feb 19, 2023

RequesterAuthorizerWithErc721 will typically be used as a cross-chain authorizer, though this doesn't affect the implementation of the Airnode-side.

@bbenligiray - just to confirm, based on the code snippet you provided above:

        "requesterAuthorizersWithErc721": [
          {
            "address": "0xCE5e...1abc",
            "erc721Address": "0xCE5e...1abc"
          }
        ]

this authorizer will be on the same chain as the chain specified by the id field of the parent chains object? In other words, it will be more like same-chain requesterEndpointAuthorizers rather than crossChainRequesterAuthorizers?

"authorizers": {
"requesterEndpointAuthorizers": ["0x1FbDB2315678afecb367f032d93F642f64180aa4"],
"crossChainRequesterAuthorizers": [
{
"requesterEndpointAuthorizers": ["0x2FbDB2315678afecb367f032d93F642f64180aa5"],
"chainType": "evm",
"chainId": "4",
"contracts": {
"AirnodeRrp": "0x5FbDB2315678afecb367f032d93F642f64180aa3"
},
"chainProvider": {
"url": "http://127.0.0.2"
}
}
]
},

@bbenligiray
Copy link
Member Author

@dcroote My bad. RequesterAuthorizerWithErc721 is intended to be deployed only on mainnet (or maybe a few select chains in the future), so my snippet only works for the mainnet chains object (and even then it may be a good idea to define it as a cross-chain authorizer for consistency). So RequesterAuthorizerWithErc721 should be usable as a cross-chain authorizer.

I attempted to fix the original example, but there are a few things I'm not sure about

  • (This was something that caught my attention before) The contracts are now called RequesterAuthorizer, so the requesterEndpointAuthorizers as a name is outdated. It should be requesterAuthorizers. This is not important by itself but makes things more confusing with the below.
  • authorizers have two possible keys, requesterEndpointAuthorizers and crossChainRequesterAuthorizers. This implies that we should add two more: requesterAuthorizersWithErc721 and crossChainRequesterAuthorizersWithErc721
  • Considering that crossChainRequesterAuthorizersWithErc721 will look quite similar to crossChainRequesterAuthorizers, maybe the part that describes the cross-chainness should be standardized

But assuming we want to wrap this up without getting bogged down, the bare minimum would be to implement a crossChainRequesterAuthorizersWithErc721 that goes under authorizers, which looks like crossChainRequesterAuthorizers except it has an Erc721 field under contracts instead of AirnodeRrp (because RequesterAuthorizersWithErc721 doesn't use https://github.com/api3dao/airnode/blob/master/packages/airnode-protocol/contracts/rrp/AuthorizationUtilsV0.sol#L27 the calls will need to batched with SelfMulticall instead).

@dcroote
Copy link
Contributor

dcroote commented Mar 8, 2023

except it has an Erc721 field under contracts instead of AirnodeRrp

@bbenligiray - having now gotten largely through implementing this I hope I interpreted it as you were thinking, or perhaps convince you if not. This new Erc721 contracts field should really be the address of RequesterAuthorizerWithErc721 and not an ERC-721 contract. The reason I interpreted it this way is that RequesterAuthorizerWithErc721 is a single, fixed, audited contract like AirnodeRrp with an analogous ability to check authorization status (isAuthorized). What is then slightly different to the existing authorizers is that a list of ERC-721 contract(s) (potentially plural) will take the place of the requesterEndpointAuthorizers array for existing authorizers. This structure also makes the authorization fetching logic flow similarly to the exiting crossChainRequesterAuthorizers.

@dcroote
Copy link
Contributor

dcroote commented Mar 8, 2023

It would have helped if I had an example of what I was describing:

 "authorizers": { 
   ...
   "crossChainRequesterAuthorizersWithErc721": [ 
     { 
       // requester authorizers (or now NFTs) are what users deploy, can have multiple
       "Erc721Authorizers": ["0x2FbDB2315678afecb367f032d93F642f64180aa5"], 
       "chainType": "evm", 
       "chainId": "4", 
       "contracts": { 
          // contracts are single things we deploy e.g. AirnodeRrp
         "RequesterAuthorizerWithErc721": "0x5FbDB2315678afecb367f032d93F642f64180aa3" 
       }, 
       "chainProvider": { 
         "url": "http://127.0.0.2" 
       } 
     } 
   ] 
 }, 

@bbenligiray
Copy link
Member Author

A RequesterAuthorizerWithErc721 contract coupled with an ERC721 contract is exactly the same as a regular RequesterAuthorizer, which is what I was trying to achieve here. This also validates your point though, a RequesterAuthorizerWithErc721 contract on its own is something more global than a regular RequesterAuthorizer. That being said, I find it as likely for someone to use multiple (customized) instances of RequesterAuthorizerWithAirnode as for them to use multiple (customized) instances of RequesterAuthorizerWithErc721, so I disagree that RequesterAuthorizerWithErc721 is comparable to AirnodeRrp in this regard.

In conclusion, I think both approaches are viable so I'll leave it up to you. One thing to note is that I would rename Erc721Authorizers to be Erc721s, as these are ordinary ERC721 contracts.

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.

3 participants