AP-763: Remove redundant imports related to AccountsCreateEndowment + fix pragma version

[0xNeshi]

Explanation of the solution

Only with these measures was I able to deploy and verify AccountsCreateEndowment with no issues on both Tenderly and Polygonscan.
Also solved the "Request Entity Too Large" error.

See difference in files included in the contract verification on Polygonscan:

• before (with just enough changes to make Polygonscan verification pass) (81 files):
  https://mumbai.polygonscan.com/address/0xDDe2b85b3f2f67c85e5B960Fc2D1bf3Bd778624E#code
• after (18 files):
  https://mumbai.polygonscan.com/address/0xE0C40e6d344Ac5975Bd08b83DEaFfD14462F28b1#code

Instructions on making this work

• run yarn or yarn install to install npm dependencies

[linear]

AP-763 Fix verification issue when verifying `AccountsCreateEndowment`

image.png

Probably pragma needs to be fixed (so no use of ^), see best practices:

https://swcregistry.io/docs/SWC-103/

[0xNeshi]

Turns out Tenderly and/or Polygonscan can compile their own version of the contract submitted for verification.
Fixing pragma version in place ensured that neither of them are able to use a compiler version different from the original.

The weirdest thing is that this compiler mismatch issue occurs randomly - for example the original AccountsCreateEndowment wasn't verifiable at all due to this error, but a slight modification would make it so, after which another modification would make it not so etc.
Even changes in this PR were causing the same issue to appear, but locking the pragma made it go away.

For more details see https://swcregistry.io/docs/SWC-103/#remediation

[0xNeshi]

@stevieraykatz @SovereignAndrey we don't have to do this for other contracts for now, but if verification fails for any of them, this might be the culprit

[stevieraykatz]

Glad it was easy to diagnose. I find it quite strange that polyscan/tenderly might use another compiler version when the later version 0.8.20+ are all unsupported by Polygon... -___-

Good find and good change

[0xNeshi]

@stevieraykatz

the later version 0.8.20+ are all unsupported by Polygon

I found the official docs that confirm that it's recommended to use <=0.8.19, but it might not mean that later versions can't be used at all to compile contracts 🤔 Maybe leaving pragma open makes these polygonscan/tenderly compilers able to use later Solidity versions nonetheless

[stevieraykatz]

The Alchemy RPCs straight up reject contracts on later versions! Though that might just be a nice feature and not reflected in other RPC validation steps.

[stevieraykatz]

Glad it was easy to diagnose. I find it quite strange that polyscan/tenderly might use another compiler version when the later version 0.8.20+ are all unsupported by Polygon... -___-

Good find and good change

[stevieraykatz]

I think this falls into the class of changes we decided we wouldn't make. Is there a reason you included them?

[0xNeshi]

This was the only way I managed to solve both:

• verification failure issue - by locking the pragma
• Request Entity Too Large error - by removing unused imports

Regarding the latter, because this is neither a state change nor a logic change (and is thus not covered by the colored graph), I created a PR for you guys to review and say whether the changes are worth including in the code base.

I should've put this rationale in the description, sorry about that.

@stevieraykatz @SovereignAndrey do you think import removal is also to be avoided (the "grey" part of the graph)?

AFAIK it doesn't affect neither state nor logic, it just reduces the compiled contract bytecode. It's also not significant enough to warrant an immediate contract redeployment and if/when it becomes necessary to redeploy a contract for whatever significant change we make to it (a contract that was at some point affected by import removal), there wouldn't be any (un)predictable side-effects.
Is my understanding correct?

[SovereignAndrey]

@stevieraykatz @misicnenad I only can speak to Cosmos in that this sort of change, removing unused imports is a change we could make, but not a critical one, so long as nothing from State was affected and it doesn't inadvertently break some referring contract. This seems to be the case here.

That said, I'm in no rush to get this on mainnet. If and when we're ready to upgrade the Accounts for something more critical, I think it's worth pushing these changes along with it, since it solves the verification issues and unblocks the SDK generation for CreateEndowment facet, makes the overall contract footprint smaller & less gas expenditures. If we decided to merge this, we should test migration of the current version of Accounts contracts to this version on local/testnet for sure. After extensive checks we might feel comfortable enough to push mainnet or as mentioned just wait for some more pressing accounts fix to make the push.

[0xNeshi]

@stevieraykatz @SovereignAndrey maybe a "compromise" solution would be to:

• focus the PR on just locking the pragma on AccountsCreateEndowment and deploy/cut the facet -> this unblocks SDK generation for said facet and we'll finally have it verified on both Tenderly and Polygonscan
• implement import removal as part of a separate issue/PR -> this can be a general import cleanup PR that we can merge with master but without rushing to redeploy all affected contracts. What @SovereignAndrey said can apply not just to facets, but to all affected contracts:

  we might... just wait for some more pressing accounts fix to make the push.

[SovereignAndrey]

@misicnenad I think your "compromise" suggested here is sound.
@stevieraykatz The final call is yours to make how you want to move forward with handling the changes covered in this PR.

[stevieraykatz]

Sorry for the delay here y'all. I agree wiht @misicnenad 's plan. The import changes can be "cut" into the existing contracts as we redeploy them for other reasons; i.e. we aren't going to plan to update them strictly for these changes. The only exception is AccountsUpdateEndowments which needs to be cut so that we can verify it.

[stevieraykatz]

The Alchemy RPCs straight up reject contracts on later versions! Though that might just be a nice feature and not reflected in other RPC validation steps.