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

Use Sourcify when encoding/decoding governance proposals #10019

Merged
merged 9 commits into from
Jan 4, 2023

Conversation

bowd
Copy link
Contributor

@bowd bowd commented Nov 17, 2022

Description

The tooling around governance proposals was initially designed under the assumption that Governance interacts only with Celo core contracts, i.e. contracts that are:

  • registered in the Registry
  • included in ContractKit
  • part of the @celo/protocol package.

This assumption was first challenged a few months ago when we needed to deploy Reserve funds to a Mobius pool, and we had to interact with external contracts. The tooling was patched in a pretty hacky fashion (#9735) due to time constraints, but the general idea was that the tooling should have additional ways to retrieve metadata about contracts it interacts with.

As we're moving forward with Mento we're moving away from the monorepo, ContractKit, and the Registry, while still having to interact with Governance for upgrades and future releases. This means that unless there's a more general solution we will hit this problem as well. And the community is also slowing moving away from a ContractKit-centric development mentality and more into broader ecosystem tools.

This PR puts forward a future-proof solution to this problem by leveraging sourcify to lookup contract metadata. Therefore governance will be able to decode/encode transactions to any contracts that are verified on Sourcify.

For example, for the Mobius pool liquidity we had this transaction as part of the cgp.json:

  {
    "contract": "0xFa3df877F98ac5ecd87456a7AcCaa948462412f0",
    "function": "addLiquidity(uint256[],uint256,uint256)",
    "args": [
      [
        "5000000000000000000000000",
        "5000000000000"
      ],
      0,
      1661419596
    ],
    "value": "0"
  }

We made this work at the time in #9735 by:
(1) Allowing addresses for the contract field instead of just Registry names.
(2) Providing a function call signature for the function field instead of just a function name
(3) Having a list of "known functions" so the tooling can turn an on-chain encoded proposal back to human readable.

(3) being a hackiest part of that PR

After this PR we can add this transaction as:

  {
    "contract": "0xFa3df877F98ac5ecd87456a7AcCaa948462412f0",
    "function": "addLiquidity",
    "args": [
      [
        "5000000000000000000000000",
        "5000000000000"
      ],
      0,
      1661419596
    ],
    "value": "0"
  }

The major difference is that we can remove the arguments from the function parameter. And the tooling will output:

0: 
  contract: SwapUSDCet(0xFa3df877F98ac5ecd87456a7AcCaa948462412f0)
  function: addLiquidity
  args: 
    0: 
      0: 5000000000000000000000000
      1: 5000000000000
    1: 0
    2: 1661419596
  params: 
    __length__: 3
    amounts: 
      0: 5000000000000000000000000
      1: 5000000000000
    minToMint: 0 
    deadline: 1661419596 (~1.661e+9)
  value: 0

What happens is that 0xFa3df877F98ac5ecd87456a7AcCaa948462412f0 is a contract that's verified on sourcify so the tooling downloads the metadata.json file from which it can extract the contract name (SwapUSDCet) and the ABI and find the addLiquidity function and encode everything with naming the params and all, without relying on any hardcoded values.

magic

The tooling also has some heuristics to determine if the contract passed in is a proxy and look up the implementation to see if it's verified. This is not very fleshed out but is isolated enough to be easily extensible.

It's important to note that if all @celo/protocol contracts would be verified on sourcify, this PR could be the first step in making the decoding/encoding part of the governance proposal much leaner.

Other changes

N/A

Tested

Added some specs for the sourcify metadata comprehension and tested out locally with different governance json configurations.

Related issues

Backwards compatibility

All CLI commands and functions intended for public use are backwards compatible. A few functions added recently and intended for internal use but accessible outside have been removed. Given there small surfaces area and unlikelihood of use this is not considered breaking

Documentation

Unsure if this requires documentation changes.

@bowd bowd requested a review from a team as a code owner November 17, 2022 13:29
@bowd bowd requested a review from gastonponti November 17, 2022 13:35
@bowd bowd assigned aaronmgdr and bowd and unassigned aaronmgdr Nov 17, 2022
@bowd bowd requested a review from aaronmgdr November 17, 2022 13:36
@bowd
Copy link
Contributor Author

bowd commented Nov 17, 2022

@aaronmgdr @gastonponti if y'all have time I'd super appreciate a review 🙏 tagged y'all 'cose you were involved in #9735

@rkachowski
Copy link
Contributor

It's important to note that if all @celo/protocol contracts would be verified on sourcify, this PR could be the first step in making the decoding/encoding part of the governance proposal much leaner.

This is something that would also be super useful for data services. We rely on sourcify's db to retrieve contract ABIs for decoding transactions.

@aaronmgdr
Copy link
Member

looks like most of the breaking changes dont really need to be breaking. the changed method names could be aliased to the new and marked as deprecated as well

Copy link
Contributor

@dckesler dckesler left a comment

Choose a reason for hiding this comment

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

I'm wondering if maybe we could update this PR to avoid the breaking changes. That would mean keeping method names the same and leaving in other methods that were deleted in this PR. It would make a release with these changes much faster and simpler.

packages/sdk/governance/src/proposals.ts Show resolved Hide resolved
packages/sdk/governance/src/proposals.ts Show resolved Hide resolved
@aaronmgdr aaronmgdr force-pushed the bowd/governance-sourcify branch from 1574ae4 to 0b71ccc Compare January 4, 2023 17:55
@aaronmgdr aaronmgdr requested a review from a team January 4, 2023 17:55
@aaronmgdr aaronmgdr force-pushed the bowd/governance-sourcify branch from c594548 to 759c20e Compare January 4, 2023 20:51
@aaronmgdr aaronmgdr merged commit 55ca063 into master Jan 4, 2023
bowd added a commit that referenced this pull request Mar 27, 2023
This is a follow-up PR to #10019, which improved the tooling around transactions targeting non-core contracts in celocli by using Sourcify to query for metadata. While testing the upcoming Mento upgrade (MU01), we realized that two pieces of missing functionality around proxy initialization are stopping us from decoding the proposal.
@mcortesi mcortesi deleted the bowd/governance-sourcify branch August 23, 2023 13:00
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.

4 participants