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

feat(ai): introduce AIServiceRegistry contract #642

Merged
merged 1 commit into from
May 5, 2024

Conversation

rickstaa
Copy link
Member

@rickstaa rickstaa commented Apr 12, 2024

What does this pull request do? Explain your changes. (required)

This pull request introduces the AIServiceRegistry contract. This contract serves as a registry for AI subnet orchestrators, enabling them to register and make their services discoverable within the AI subnet.

Specific updates (required)

  • A deployment script to deploy a new ServiceRegistry contract under the AIServiceRegistry name was added.
  • Information about the deployed contracts was commited to the deployments directory.

How did you test each of these updates (required)

I ensured that all tests passed. Deployed the contract and interacted with the contract using foundry cast.

Test Deployments

EDIT: We decide to just deploy a target contract connected to the mainnet controller right now.

Does this pull request close any open issues?

https://linear.app/livepeer-ai-spe/issue/LIV-90/design-and-implement-solution-for-advertising-separate-service-uri-for

Checklist:

  • README and other documentation updated
  • All tests using yarn test pass

@rickstaa
Copy link
Member Author

rickstaa commented Apr 12, 2024

@yondonfu there are multiple ways to do this. I now created a seperate AIServiceRegistry contract that inherits all properties from the regular ServiceRegistry contract but we can also keep one ServiceRegistry contract and deploy the AIServiceRegistry contract based on that.

I've deployed it using my own developer wallet, along with a brand new controller contract. However, it might be more effective if someone else on the team handles the deployment so that it can be directly linked to the main controller contract. One potential issue with this approach is the absence of a RemoveContractInfo method in the controller contract, as far as I am aware. This means we might only be able to reset it to a default setting should we decide to consolidate the two Service Registries in the future 🤔.

@rickstaa
Copy link
Member Author

@yondonfu I also noticed although the proxy contract works automatic verification seems to fail for this contract (see https://arbiscan.io/address/0x73f1755f34c2e769209e0f91a0c385722ed81b9c#code). Are you familiar with this problem?

@rickstaa rickstaa requested a review from yondonfu April 13, 2024 00:17
@rickstaa rickstaa marked this pull request as draft April 13, 2024 00:17
Copy link
Member

@yondonfu yondonfu left a comment

Choose a reason for hiding this comment

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

there are multiple ways to do this. I now created a seperate AIServiceRegistry contract that inherits all properties from the regular ServiceRegistry contract but we can also keep one ServiceRegistry contract and deploy the AIServiceRegistry contract based on that.

IMO the latter option would be simpler - if we just deploy another instance of the ServiceRegistry code and refer to it as the AIServiceRegistry that would remove the need for additional tests or custom code that specifically references AIServiceRegistry by name.

deploy/deploy_ai_service_registry.ts Show resolved Hide resolved
deploy/deploy_contracts.ts Outdated Show resolved Hide resolved
@yondonfu
Copy link
Member

I also noticed although the proxy contract works automatic verification seems to fail for this contract

If a contract is a proxy, you would have to use the arbiscan UI to confirm it is a proxy, make sure it is pointing at the right target implementation address.

Screenshot 2024-04-15 at 2 47 14 PM

Though, as mentioned in my previous comment, if a non-proxy version of the ServiceRegistry is deployed as the AIServiceRegistry then won't have to deal with proxy verification on arbiscan.

@rickstaa
Copy link
Member Author

I also noticed although the proxy contract works automatic verification seems to fail for this contract

If a contract is a proxy, you would have to use the arbiscan UI to confirm it is a proxy, make sure it is pointing at the right target implementation address.

Screenshot 2024-04-15 at 2 47 14 PM

Though, as mentioned in my previous comment, if a non-proxy version of the ServiceRegistry is deployed as the AIServiceRegistry then won't have to deal with proxy verification on arbiscan.

Yea looks like the target is verified automatically causing the automatic Proxy verification on Arbitrum explorer to fail 🤔. @iameli stated that he also had problems with that some time ago and was able to get it verified manually. However let's for now just simply things by deploying a non-proxy version 👍🏻.

rickstaa added a commit that referenced this pull request Apr 17, 2024
This commit introduces the `AIServiceRegistry` contract. This contract
serves as a registry for AI subnet orchestrators, enabling them to
register and make their services discoverable within the AI subnet. As
discussed in #642 we opted to
deploy a non-upgradable contract for now connected to LivePeer's
[mainnet controller](https://docs.livepeer.org/references/contract-addresses).
This commit adds the AIServiceRegistry contract, facilitating the
registration and discoverability of AI subnet orchestrators'
services. As discussed in #642, we've chosen to deploy a
non-upgradable contract for now, linked to LivePeer's mainnet
controller (https://docs.livepeer.org/references/contract-addresses).
@rickstaa rickstaa marked this pull request as ready for review April 17, 2024 17:17
@rickstaa
Copy link
Member Author

@yondonfu, I've re-deployed the contract as an immutable standalone contract. The potential drawback is that if we decide to register this contract with the controller for future upgrades, similar to the standard ServiceRegistry contract, we would need to deploy a new contract. This change would require us to either migrate the existing service URIs to the new contract (though it’s unclear if this is feasible) or compel orchestrators to resubmit their URIs. It's manageable, but worth considering. Additionally, I've set up #643, which, from my understanding, would allow us to register both the Proxy and target with the controller 🤔. I'm fine with using any of the two versions 👍🏻.

@rickstaa rickstaa marked this pull request as draft April 19, 2024 00:11
@yondonfu
Copy link
Member

@rickstaa IMO using an immutable standalone contract at this stage would be preferable because deploying an upgradeable proxy involves additional complexity:

  • If the AIServiceRegistry is deployed using the ManagerProxy pattern (which is how all core protocol contracts are deployed), then the Governor would be the only address that is able to upgrade the implementation.
  • If the AIServiceRegistry is deployed with another upgradeable proxy pattern, then there is still some admin address that needs to be defined that would be able to upgrade the implementation.

In both cases, the operational setup for the contract becomes more complex and I think the tradeoff of potentially having to migrate service URIs to another contract in the future is reasonable to avoid that extra complexity especially since the path to "merging" the AI subnet and the transcoding mainnet at this point is still unclear.

@rickstaa
Copy link
Member Author

@rickstaa IMO using an immutable standalone contract at this stage would be preferable because deploying an upgradeable proxy involves additional complexity:

  • If the AIServiceRegistry is deployed using the ManagerProxy pattern (which is how all core protocol contracts are deployed), then the Governor would be the only address that is able to upgrade the implementation.
  • If the AIServiceRegistry is deployed with another upgradeable proxy pattern, then there is still some admin address that needs to be defined that would be able to upgrade the implementation.

In both cases, the operational setup for the contract becomes more complex and I think the tradeoff of potentially having to migrate service URIs to another contract in the future is reasonable to avoid that extra complexity especially since the path to "merging" the AI subnet and the transcoding mainnet at this point is still unclear.

@yondonfu, thank you for your response; it is helpful. Ultimately, I decided to proceed with the immutable standalone contract (i.e. livepeer/go-livepeer#3004).

@yondonfu
Copy link
Member

yondonfu commented May 2, 2024

@rickstaa If this is deployed and being used already suggest moving this out of draft so it can be properly reviewed + merged.

@rickstaa
Copy link
Member Author

rickstaa commented May 4, 2024

@rickstaa If this is deployed and being used already suggest moving this out of draft so it can be properly reviewed + merged.

Great idea! I wasn't sure if I should keep it as a draft until we merge with Mainnet but happy to move it out of draft 👍🏻.

@rickstaa rickstaa marked this pull request as ready for review May 4, 2024 10:21
@rickstaa rickstaa merged commit 75d0b32 into delta May 5, 2024
5 checks passed
@rickstaa rickstaa deleted the add_ai_service_registery branch May 5, 2024 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants