celo-org / celo-monorepo

Official repository for core projects comprising the Celo platform
https://celo.org
Apache License 2.0
687 stars 362 forks source link

Add ReserveSpenderMultiSig to the foundry migrations #11025

Closed arthurgousset closed 1 month ago

arthurgousset commented 1 month ago

From @shazarre:

reserve tests are failing with MultiSig not (yet) registered

 ● reserve:transfergold cmd › can transferGold with multisig option

    MultiSig not (yet) registered

Source: Slack

shazarre commented 1 month ago

Exact output is:

 FAIL  src/commands/reserve/transfergold.test.ts
  reserve:transfergold cmd
    ✓ transferGold fails if spender not passed in (723 ms)
    ✕ can transferGold with multisig option (45 ms)

  ● reserve:transfergold cmd › can transferGold with multisig option

    MultiSig not (yet) registered

      41 |       debug('Fetched address %s', address)
      42 |       if (!address || address === NULL_ADDRESS) {
    > 43 |         throw new UnregisteredError(contract)
         |               ^
      44 |       }
      45 |       this.cache.set(contract, address as StrongAddress)
      46 |     }

while the test can be found here.

arthurgousset commented 1 month ago

For reference, the test is:

test('can transferGold with multisig option', async () => {
    const initialBalance = await goldToken.balanceOf(accounts[9])
    await testLocally(TransferGold, [
      '--from',
      accounts[0],
      '--value',
      transferAmt.toString(10),
      '--to',
      accounts[9],
      '--useMultiSig',
    ])
    await testLocally(TransferGold, [
      '--from',
      accounts[7],
      '--value',
      transferAmt.toString(10),
      '--to',
      accounts[9],
      '--useMultiSig',
    ])
    expect(await goldToken.balanceOf(accounts[9])).toEqual(initialBalance.plus(transferAmt))
  })
arthurgousset commented 1 month ago

There is a comment relating to this in the migrations:

https://github.com/celo-org/celo-monorepo/blob/b10b77a5b3b1ac9e8750b246e786a649548a2556/packages/protocol/migrations_sol/Migration.s.sol#L332-L333

arthurgousset commented 1 month ago

Looks like this might be the related ganache migration:

https://github.com/celo-org/celo-monorepo/blob/b10b77a5b3b1ac9e8750b246e786a649548a2556/packages/protocol/migrations_ts/07_reserve_spender_multisig.ts#L11-L32

The relevant configs are:

https://github.com/celo-org/celo-monorepo/blob/b10b77a5b3b1ac9e8750b246e786a649548a2556/packages/protocol/migrationsConfig.js#L163-L167

arthurgousset commented 1 month ago

Here is a similar MultiSig deployment:

In foundry:

https://github.com/celo-org/celo-monorepo/blob/b10b77a5b3b1ac9e8750b246e786a649548a2556/packages/protocol/migrations_sol/Migration.s.sol#L787-L808

https://github.com/celo-org/celo-monorepo/blob/b10b77a5b3b1ac9e8750b246e786a649548a2556/packages/protocol/migrations_sol/migrationsConfig.json#L132-L136

In ganache:

https://github.com/celo-org/celo-monorepo/blob/b10b77a5b3b1ac9e8750b246e786a649548a2556/packages/protocol/migrations_ts/23_governance_approver_multisig.ts#L9-L29

https://github.com/celo-org/celo-monorepo/blob/b10b77a5b3b1ac9e8750b246e786a649548a2556/packages/protocol/migrationsConfig.js#L114-L120

arthurgousset commented 1 month ago

Working on the PR here:

arthurgousset commented 1 month ago

For visibility @shazarre, I marked this PR as ready for review and asked for a review from the primitive team:

If and when it's merged to master, the new devchain will automatically publish, and you can let me know if the CLI test passes as expected.

arthurgousset commented 1 month ago

@shazarre this is done, and the new @celo/devchain-anvil is currently being published (see CI).

Let me know if the CLI test passes as expected.

arthurgousset commented 1 month ago

@arthurgousset created a branch that has failing examples for both releasecelo and reserve

to test it:

git checkout shazarre/Migrate_reserve_and_releasecelo_tests_to_anvil
yarn && yarn build
yarn workspace @celo/celocli run test

Originally posted by @shazarre in https://github.com/celo-org/celo-monorepo/issues/11026#issuecomment-2161124727

arthurgousset commented 1 month ago

When I run the tests as described above, I get this error:

 FAIL  src/commands/reserve/transfergold.test.ts
  ● Console

    console.log
      Running Checks:

      at CheckBuilder.runChecks (src/utils/checks.ts:491:13)

    console.log
         ✘  0x5409ED021D9299bf6814279A6A1411A7e866A631 is a reserve spender

      at CheckBuilder.runChecks (src/utils/checks.ts:498:15)

    console.log
         ✘  0x91c987bf62D25945dB517BDAa840A6c661374402 is another reserve address
arthurgousset commented 1 month ago

It looks like the CLI doesn't find any spenders using the getSpenders() helper function:

async getSpenders(): Promise<Address[]> {
    const spendersAdded = (
      await this.getPastEvents('SpenderAdded', {
        fromBlock: 0,
        toBlock: 'latest',
      })
    ).map((eventlog: EventLog) => eventlog.returnValues.spender)
    const spendersRemoved = (
      await this.getPastEvents('SpenderRemoved', {
        fromBlock: 0,
        toBlock: 'latest',
      })
    ).map((eventlog: EventLog) => eventlog.returnValues.spender)
    return spendersAdded.filter((spender) => !spendersRemoved.includes(spender))
  }
}

Source: contractkit/src/wrappers/Reserve.ts

const spenders = useMultiSig ? await reserve.getSpenders() : []
/////////////////////// DEBUGGING ///////////////////////
console.log('spenders[] are:', spenders)
/////////////////////// DEBUGGING ///////////////////////

Source: cli/src/commands/reserve/transfergold.ts

git checkout shazarre/Migrate_reserve_and_releasecelo_tests_to_anvil
yarn && yarn build
yarn workspace @celo/celocli run test

console.log
  spenders[] are: []

  at TransferGold.run (src/commands/reserve/transfergold.ts:36:13)

On the devchain, the SpenderAdded event is emitted:

cast logs \
"SpenderAdded(address indexed spender)" \
--from-block 0 --to-block latest \
--rpc-url=http://127.0.0.1:8546
- address: 0x0a887500E327375378edF7b7d0E85F0378b8677a
  blockHash: 0xdd3366edde3433c624cf059c190e864470ee5ed423fe57393fabffc52020c242
  blockNumber: 43
  data: 0x
  logIndex: 0
  removed: false
  topics: [
    0x3139419c41cdd7abca84fa19dd21118cd285d3e2ce1a9444e8161ce9fa62fdcd
    0x0000000000000000000000001a888d93eeacf683c68e07ad58aa43f2a5742490
  ]
  transactionHash: 0xbf4e79a92f7c6a1be5451d1489e70278efd89c9c15c3497e968527d854f6063c
  transactionIndex: 0

The ReserveSpenderMultiSig address is 0x1A888D93eeAcF683c68E07Ad58Aa43f2A5742490 as expected from the log and the contract call.

cast call \
"0x0a887500E327375378edF7b7d0E85F0378b8677a" \
"isSpender(address)(bool)" \
"0x1A888D93eeAcF683c68E07Ad58Aa43f2A5742490" \
--rpc-url=http://127.0.0.1:8546
true
arthurgousset commented 1 month ago

Minimal reproducible example to check if the bug is related to the JSON dump or NPM version

  1. Make a test directory

    mkdir ~/Documents/celo-org/devchain-anvil-installation
    cd ~/Documents/celo-org/devchain-anvil-installation
  2. Install the @celo/devchain-anvil package

    npm install --save-dev @celo/devchain-anvil
  3. Start up anvil with the JSON state from the package

    anvil --state node_modules/@celo/devchain-anvil/devchain.json
  4. (separate terminal) Find the Reserve address in the Registry

    cast call \
    "0x000000000000000000000000000000000000ce10" \
    "getAddressForStringOrDie(string calldata identifier)(address)" \
    "Reserve" \
    --rpc-url=http://127.0.0.1:8545
    0xe5cC39E5404c451604A85a4CD9686d918D18577e
  5. (separate terminal) Find the ReserveSpenderMultiSig address in the Registry

    cast call \
    "0x000000000000000000000000000000000000ce10" \
    "getAddressForStringOrDie(string calldata identifier)(address)" \
    "ReserveSpenderMultiSig" \
    --rpc-url=http://127.0.0.1:8545
    0x909158d6b441312D6813CBea21D466C66ef94280
  6. (separate terminal) Check that 0x1A888D93eeAcF683c68E07Ad58Aa43f2A5742490 is a spender ✅

    cast call \
    "0xe5cC39E5404c451604A85a4CD9686d918D18577e" \
    "isSpender(address)(bool)" \
    "0x909158d6b441312D6813CBea21D466C66ef94280" \
    --rpc-url=http://127.0.0.1:8545
    true
  7. But, we found that logs are not included in the JSON dump. That means helpers that use events, cannot be used anymore with Anvil. ❌

    cast logs \
    "SpenderAdded(address indexed spender)" \
    --from-block 0 --to-block latest \
    --rpc-url=http://127.0.0.1:8545

    Anvil cannot dump events into a JSON. This is a known limitation:

arthurgousset commented 1 month ago

That means the contractkit/src/wrappers/Reserve.ts helper function won't work with Anvil.

But, there is an (anvil) devchain specific fix, which is to get the ReserveSpenderMultiSig address from the Registry. The contract is in the Registry on the devchain for ease of reference, but it's not available on Mainnet.

arthurgousset commented 1 month ago

I'm closing this issue on that basis.