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

Implement RequesterAuthorizerWithErc721 authorizers #1670

Merged
merged 15 commits into from
Mar 17, 2023
Merged

Implement RequesterAuthorizerWithErc721 authorizers #1670

merged 15 commits into from
Mar 17, 2023

Conversation

dcroote
Copy link
Contributor

@dcroote dcroote commented Mar 13, 2023

Closes #1632 by adding two new authorizers (same chain and cross chain). In each, RequesterAuthorizerWithErc721 corresponds to the address of the authorizer contract we deploy (from here, incorporated completely herein along with other necessary contracts from airnode-protocol-v1), and erc721s is an array of NFTs that serve as tokens to authorize requesters.

        "requesterAuthorizersWithErc721": [
          {
            "erc721s": ["0x00bDB2315678afecb367f032d93F642f64180a00"],
            "RequesterAuthorizerWithErc721": "0x999DB2315678afecb367f032d93F642f64180aa9"
          }
        ],
        "crossChainRequesterAuthorizersWithErc721": [
          {
            "erc721s": ["0x3FbDB2315678afecb367f032d93F642f64180aa6"],
            "chainType": "evm",
            "chainId": "4",
            "contracts": {
              "RequesterAuthorizerWithErc721": "0x6bbbb2315678afecb367f032d93F642f64180aa4"
            },
            "chainProvider": {
              "url": "http://127.0.0.2"
            }
          }
        ]

@bbenligiray I'm still struggling a bit on the e2e test, see the second commit

@Siegrift
Copy link
Contributor

I've made the e2e test work (hacky way) in 950f5ec

The proper implementation requires a small refactor.

@bbenligiray
Copy link
Member

👍 for the config format

@dcroote
Fix authorizers empty arrays behavior
beb4484 
Previously requesterEndpointAuthorizers being empty would result in all
requests being authorized, independent of whether the other authorizer
arrays were nonempty
@dcroote
Copy link
Contributor Author

dcroote commented Mar 14, 2023

@Siegrift thanks for such a huge help, I really appreciate it.

I've made the e2e test work

Unfortunately due to a legacy subtlety in authorizer.requesterEndpointAuthorizers empty array behavior, your request was being authorized by this authorizer instead of the new requesterAuthorizersWithErc721 authorizer. See my last commit (in which I added a check for all authorizers being empty). I also changed dependencies, see my first commit after yours.

For isAuthorized() within this new authorizer, it appears an NFT (token) has to be deposited... Still working through understanding that

@Siegrift
Copy link
Contributor

For isAuthorized() within this new authorizer, it appears an NFT (token) has to be deposited... Still working through understanding that

Yeah, sorry. I mainly focused on how to make the deployments (NFT and the respective authorizer) work. I thought that was the challenging part. Anyway, I think know how to make it work with token deposit. I'll try to fix the test today.

@Siegrift
Copy link
Contributor

@dcroote Take a look at b3db6aa

@dcroote dcroote marked this pull request as ready for review March 15, 2023 05:16
@dcroote dcroote requested a review from Siegrift March 15, 2023 05:17
@dcroote dcroote self-assigned this Mar 15, 2023
@dcroote dcroote requested a review from a team March 15, 2023 05:38
Siegrift
Siegrift requested changes Mar 15, 2023
View reviewed changes
packages/airnode-node/src/evm/authorization/authorization-application.test.ts
expect(logs).toEqual([
{
level: 'DEBUG',
message: 'Requester:requesterAddress is authorized to access Endpoint ID:endpointId for Request ID:0xapiCallId',
Copy link
Contributor

Choose a reason for hiding this comment

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

It would also be nice debug level info which requester authorized the given call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We previously had no logging when a request was authorized, so even this is an upgrade 😅. I looked into adding the originating authorizer to the log, but that is not trivial to do based on how authorization fetching currently occurs. I think it's doable, but outside the scope of this PR. Happy to open a separate issue though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I feel you. We can open an issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#1679

packages/airnode-node/src/evm/authorization/authorization-fetching.test.ts Show resolved Hide resolved
packages/airnode-node/src/evm/handlers/initialize-provider.ts Outdated Show resolved Hide resolved
packages/airnode-node/src/evm/handlers/initialize-provider.ts Show resolved Hide resolved
packages/airnode-protocol/contracts/authorizers/mock/MockRequesterAuthorizerWithErc721.sol Outdated Show resolved Hide resolved
packages/airnode-protocol/contracts/authorizers/mock/MockRequesterAuthorizerWithErc721.sol Outdated
// SPDX-License-Identifier: MIT
pragma solidity 0.8.17;

import "../../dev/RequesterAuthorizerWithErc721.sol";
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we want to copy the contents to the Airnode repo or use it as a dependency. Dependency soundsw better to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Burak suggested copy the contents:https://api3workspace.slack.com/archives/C0436H2BJKE/p1677057747622019?thread_ts=1676612819.041809&cid=C0436H2BJKE

Copy link
Member

Choose a reason for hiding this comment

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

I can temporarily publish https://github.com/api3dao/nft-authorizer separately if you want, but to me these feel like equivalent solutions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was convinced by the argument that these will eventually be in this repo anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, if it's going to be moved I am ok with it. An issue in the Airnode repo would be nice though.

Copy link
Member

Choose a reason for hiding this comment

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

#1675

packages/airnode-validator/src/config/config.ts Outdated Show resolved Hide resolved
packages/airnode-validator/src/config/config.ts Outdated Show resolved Hide resolved
packages/airnode-validator/src/config/config.ts Outdated Show resolved Hide resolved
@dcroote dcroote requested a review from Siegrift March 16, 2023 06:39
@bbenligiray bbenligiray mentioned this pull request Mar 16, 2023
Closed
Siegrift
Siegrift approved these changes Mar 16, 2023
View reviewed changes
Copy link
Contributor

@Siegrift Siegrift left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@dcroote dcroote merged commit 1b486bb into master Mar 17, 2023
@dcroote dcroote deleted the dcroote/issue1632 branch March 17, 2023 05:08
@dcroote dcroote mentioned this pull request Mar 17, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bbenligiray bbenligiray bbenligiray left review comments

@Siegrift Siegrift Siegrift approved these changes

Assignees

@dcroote dcroote

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement RequesterAuthorizerWithErc721 support
3 participants
@dcroote @Siegrift @bbenligiray