Skip to content
This repository has been archived by the owner on Mar 12, 2024. It is now read-only.

Add Authority factory #64

Closed
wants to merge 3 commits into from
Closed

Conversation

pedroargento
Copy link

@pedroargento pedroargento commented May 27, 2023

resolves cartesi/rollups-contracts#25
Changes:

  • Add a AuthorityFactory (and interface) smart contract to deploy Authority contracts.
  • Add tests to AuthorityFactory.

TODO:

  • Add tests for emitted events.
  • Generate output proofs.

@pedroargento
Copy link
Author

I had trouble generating the output proofs on my machine because of some conflicting node versions.

@ZzzzHui
Copy link
Contributor

ZzzzHui commented May 29, 2023

I created a PR to suggest some changes :)


contract AuthorityFactoryTest is TestBase {
AuthorityFactory factory;
InputBox inputBox;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should make this state variable typed as IInputBox. :-)

@tuler
Copy link
Member

tuler commented May 29, 2023

What is the plan for History setup?

@guidanoli
Copy link
Contributor

What is the plan for History setup?

@tuler I would suggest we create a factory for each history we implement. Currently, there is only one history implementation, which only accepts individual claims, but we envision other history implementations in the future. This would allow for optimal modularity: you can deploy any kind of consensus, which uses any kind of history.

What do you think? FYI @DaniOrtegaB @pedroargento @ZzzzHui

@tuler
Copy link
Member

tuler commented May 30, 2023

@guidanoli I was hoping to replace the code below with a single transaction.

    const Authority = await deployments.deploy("SunodoAuthority", {
        ...opts,
        contract: "Authority",
        args: [deployer, InputBox.address],
    });

    const History = await deployments.deploy("SunodoHistory", {
        ...opts,
        contract: "History",
        args: [Authority.address],
    });

    const authority = Authority__factory.connect(Authority.address, signer);
    const currentHistory = await authority.getHistory();
    if (currentHistory != History.address) {
        const tx = await authority.setHistory(History.address);
        const receipt = await tx.wait();
        if (receipt.status == 0) {
            throw Error(`Could not link Authority to history: ${tx.hash}`);
        }
    }

@guidanoli
Copy link
Contributor

@guidanoli I was hoping to replace the code below with a single transaction.

For this (very) common case, where we use Authority linked to a simple History, we could create a contract that does all that. It would receive both factory addresses on the constructor and make them immutable (embedding them in the final byte code), and have an entry point that would instantiate an Authority and a History contract, and link them. We could even have a deterministic deployment that uses the same salt. What do you think?

@pedroargento
Copy link
Author

If we create a History factory maybe its time to make a better abstraction for what a factory is. The Dapp Factory and Authority Factory share a lot of code.

@guidanoli
Copy link
Contributor

guidanoli commented May 30, 2023

If we create a History factory maybe its time to make a better abstraction for what a factory is. The Dapp Factory and Authority Factory share a lot of code.

Unfortunately, I don't think we are able to create these high-level typed abstractions in Solidity like you probably could in compilation-time-heavy languages like C++ or Rust. For example, defining a Factory<T> generic contract is not possible in the latest version of Solidity (0.8.20).

@pedroargento
Copy link
Author

I see, it makes sense... So the only thing we could repurpose would be the deterministic address calculation, right?

@guidanoli
Copy link
Contributor

I see, it makes sense... So the only thing we could repurpose would be the deterministic address calculation, right?

Oh, you're right. We could create a library of some sorts that has this deterministic address calculation routine. Let us see also if there is anything like these already implemented so we don't need to reinvent the wheel. :-)

@guidanoli
Copy link
Contributor

@pedroargento It seems like OpenZeppelin has a Create2 library we can use. :-)

@guidanoli guidanoli changed the title Feature/authority factory Add Authority factory May 30, 2023
@tuler
Copy link
Member

tuler commented Jul 6, 2023

I think it's easier to wire the InputBox in the factory instead of passing as an arg to new newAuthority

@guidanoli guidanoli force-pushed the feature/authority-factory branch 2 times, most recently from 2fa670e to 12b743c Compare July 11, 2023 13:33
@guidanoli
Copy link
Contributor

guidanoli commented Jul 11, 2023

Rebased and fixed commit scopes (onchain-contracts ➡️ contracts).

@guidanoli
Copy link
Contributor

Ran yarn prepack.

@guidanoli
Copy link
Contributor

Rebased and organized commits.

@guidanoli
Copy link
Contributor

This PR has been migrated to cartesi/rollups-contracts#26.

@guidanoli guidanoli closed this Aug 26, 2023
@guidanoli guidanoli deleted the feature/authority-factory branch August 28, 2023 16:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Authority factory
4 participants