search

livepeer / protocol

Livepeer protocol
MIT License
152 stars 45 forks source link

feat(ai): introduce AIServiceRegistry contract #642

Closed rickstaa closed 6 months ago

rickstaa commented 7 months ago

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)

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:

rickstaa commented 7 months ago

@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 commented 7 months ago

@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?

yondonfu commented 7 months ago

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 commented 7 months ago

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 commented 6 months ago

@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 https://github.com/livepeer/protocol/pull/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 👍🏻.

yondonfu commented 6 months ago

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

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 commented 6 months ago

@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. https://github.com/livepeer/go-livepeer/pull/3004).

yondonfu commented 6 months ago

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

rickstaa commented 6 months ago

@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 👍🏻.