🐞 Fix TrueFiPool2 'tether flush' test and disable Certora in CI

[pkuchtatt]

No description provided.

[pkuchtatt]

I don't understand why the CircleCI verify task is failing again, it was removed recently: #1178

I will remove it again in this PR if that's OK, @yuchenlintt, @tt-krzysztof .

[yuchenlintt]

I don't fully understand the fix. Could you clarify what the new PROXY_ADDRESS is supposed to do, and where it comes from?

The current proxy points to our deployed tfUSDT contract proxy on mainnet:
https://etherscan.io/address/0x6002b1dcb26e7b1aa797a17551c6f487923299d7

The purpose of this test is to ensure that upgrades to this contract don't cause a regression that makes the flush() function revert. I believe this previously caused a bug that made it impossible for users to deposit or withdraw from the pool, so we want to make sure that any future changes we submit don't break this.

That said, I believe we've currently disabled curve flushing, so maybe this test can be deleted altogether?

[yuchenlintt]

Where does this address come from? I don't see it on Etherscan:
https://etherscan.io/address/0x0BA5908a77f2536b11a88d162CAa649E981E2a0b

[pkuchtatt]

The address is made up. I didn't think it mattered what it was, I thought the test worked with locally running Ganache rather than Ethereum mainnet. The problem I was trying to address was that we used the same address for two different contracts, see lines number 48 and 49. It was:

    const usdtPool = TrueFiPool2__factory.connect(TFUSDT_ADDRESS, powner)
    const proxy = OwnedProxyWithReference__factory.connect(TFUSDT_ADDRESS, powner)

[yuchenlintt]

This is a perfect learning moment, since we also want to write one of these "integration" tests for the new TrueUSDNoDelegate. (You can also see a minimal example in PR #1177, which also should be reviewed + merged at some point -- the action for which we wrote that test has already been queued and executed on the multisig.)

We use Ganache locally to run the test on a mainnet fork. We fork mainnet in the forkChain() call in line 27. You can optionally specify:

• unlocked account addresses with particular properties (e.g., the owner() or proxyOwner() of the contract, or ETH_HOLDER is just a mainnet address that has a bunch of tokens), and
• the block number to fork, in case there's some specific past contract state that we want to test.

[yuchenlintt]

Using the same address is a hacky workaround meant specifically for proxies. A proxy only supports a couple native functions, like proxyOwner(), transferProxyOwnership(), upgradeTo(), etc. Every other selector is forwarded on-chain to the implementation address's ABI.

Unfortunately Ganache and Ethers don't know this, so they complain whenever we issue a tx to a proxy that doesn't match its native ABI.

To work around this limitation when we need to make both upgrade and impl calls in the same test, we can connect to the same proxy address with its proxy ABI and its impl ABI as "separate" contracts.

[yuchenlintt]

I will remove it again in this PR if that's OK, @yuchenlintt, @tt-krzysztof .

Yeah, that's fine! In that case, let's change the name of this PR to include it.

[pkuchtatt]

That said, I believe we've currently disabled curve flushing, so maybe this test can be deleted altogether?

I will be more than happy to delete the test if you tell me it's no longer needed. Let's do it and let's make sure we don't merge anything without making sure CI passes in the future.

[yuchenlintt]

I will be more than happy to delete the test if you tell me it's no longer needed. Let's do it and let's make sure we don't merge anything without making sure CI passes in the future.

On second examination, I think the test is still important to keep. flush() is currently disabled for everyone except the contract owner (CONFIG_MULTISIG in this test), but the validity of its behavior should still be checked.

Now that I reflect on it, I suspect what's causing test failure is the recent governance proposal that claimed proxy ownership of tfUSDT for the DAO TimelockController:
https://www.tally.xyz/governance/eip155:1:0x585CcA060422ef1779Fb0Dd710A49e7C49A823C9/proposal/32137011153032438281350136128034661202026790869146491991828132207554601395947

If that's the case, then what we want to do is add another unlocked address for the DAO TimelockController:

const DAO_TIMELOCK_CONTROLLER = '0x4f4AC7a7032A14243aEbDa98Ee04a5D7Fe293d07`

Make sure it gets unlocked in forkChain([DAO_TIMELOCK_CONTROLLER, OWNER, ...]), and then assigned to a signer:

const daoTimelockController = provider.getSigner(DAO_TIMELOCK_CONTROLLER)

And then instead of connecting to proxy with the old powner, we should connect with daoTimelockController:

const proxy = OwnedProxyWithReference__factory.connect(PROXY_ADDRESS, daoTimelockController)

Let's see if this fixes the test? 🤞

[yuchenlintt]

This is a perfect learning moment, since we also want to write one of these "integration" tests for the new TrueUSDNoDelegate. (You can also see a minimal example in PR #1177, which also should be reviewed + merged at some point -- the action for which we wrote that test has already been queued and executed on the multisig.)

We use Ganache locally to run the test on a mainnet fork. We fork mainnet in the forkChain() call in line 27. You can optionally specify:

• unlocked account addresses with particular properties (e.g., the owner() or proxyOwner() of the contract, or ETH_HOLDER is just a mainnet address that has a bunch of tokens), and
• the block number to fork, in case there's some specific past contract state that we want to test.

[yuchenlintt]

Using the same address is a hacky workaround meant specifically for proxies. A proxy only supports a couple native functions, like proxyOwner(), transferProxyOwnership(), upgradeTo(), etc. Every other selector is forwarded on-chain to the implementation address's ABI.

Unfortunately Ganache and Ethers don't know this, so they complain whenever we issue a tx to a proxy that doesn't match its native ABI.

To work around this limitation when we need to make both upgrade and impl calls in the same test, we can connect to the same proxy address with its proxy ABI and its impl ABI as "separate" contracts.

[yuchenlintt]

LGTM! Assuming the test passes, feel free to merge this PR.

[pkuchtatt]

LGTM! Assuming the test passes, feel free to merge this PR.

The test passes for me locally, unfortunately there is a timeout in CI. I have seen this before few times, I believe this is nondeterministic. I'll push a noop change to see if the timeout goes away ^H^H^H^H^H^H^H^H^H^H^H
Restarted the tests in CircleCI to see if the timeout happens again.

[pkuchtatt]

After restarting CI all tests passed! Need another approval to merge, though. @tt-krzysztof Can you please approve the PR? Or should I ask someone else, @yuchenlintt?