Skip to content
This repository has been archived by the owner on Oct 6, 2023. It is now read-only.

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

Merged
merged 3 commits into from
Sep 26, 2023
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1b78a34
Remove redundant imports related to AccountsCreateEndowment + fix pragma
0xNeshi Sep 20, 2023
37056e5
Revert "Remove redundant imports related to AccountsCreateEndowment +…
0xNeshi Sep 26, 2023
a6e5698
Fix pragma in AccountsCreateEndowment
0xNeshi Sep 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion contracts/core/accounts/facets/AccountsCreateEndowment.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;
pragma solidity 0.8.19;
Copy link
Contributor Author

@0xNeshi 0xNeshi Sep 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@0xNeshi 0xNeshi Sep 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


import {LibAccounts} from "../lib/LibAccounts.sol";
import {Validator} from "../../validator.sol";
Expand Down
1 change: 0 additions & 1 deletion contracts/core/accounts/storage.sol
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@0xNeshi 0xNeshi Sep 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@SovereignAndrey SovereignAndrey Sep 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

@0xNeshi 0xNeshi Sep 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

@SovereignAndrey SovereignAndrey Sep 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
pragma solidity ^0.8.19;

import {LibAccounts} from "./lib/LibAccounts.sol";
import {LocalRegistrarLib} from "../registrar/lib/LocalRegistrarLib.sol";
import {IterableMappingAddr} from "../../lib/IterableMappingAddr.sol";
import {IterableMappingStrategy} from "../../lib/IterableMappingStrategy.sol";
import {IVault} from "../vault/interfaces/IVault.sol";
Expand Down
2 changes: 0 additions & 2 deletions contracts/core/gasFwd/IGasFwdFactory.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;

import {ProxyContract} from "../proxy.sol";

interface IGasFwdFactory {
error InvalidAddress(string param);
event GasFwdCreated(address addr);
Expand Down
1 change: 0 additions & 1 deletion contracts/core/registrar/interfaces/ILocalRegistrar.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ pragma solidity ^0.8.19;

import {LocalRegistrarLib} from "../lib/LocalRegistrarLib.sol";
import {LibAccounts} from "../../accounts/lib/LibAccounts.sol";
import {IAccountsStrategy} from "../../accounts/interfaces/IAccountsStrategy.sol";

interface ILocalRegistrar {
/*////////////////////////////////////////////////
Expand Down
1 change: 0 additions & 1 deletion contracts/core/registrar/lib/LocalRegistrarLib.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// author: @stevieraykatz
pragma solidity ^0.8.19;

import {IVault} from "../../vault/interfaces/IVault.sol";
import {LibAccounts} from "../../accounts/lib/LibAccounts.sol";

library LocalRegistrarLib {
Expand Down
1 change: 0 additions & 1 deletion contracts/core/registrar/message.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
pragma solidity ^0.8.19;

import {LibAccounts} from "../accounts/lib/LibAccounts.sol";
import {IVault} from "../vault/interfaces/IVault.sol";

library RegistrarMessages {
struct InstantiateRequest {
Expand Down
3 changes: 0 additions & 3 deletions contracts/core/registrar/storage.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;

import {LibAccounts} from "../accounts/lib/LibAccounts.sol";
import {LocalRegistrarLib} from "./lib/LocalRegistrarLib.sol";

library RegistrarStorage {
struct Config {
address accountsContract;
Expand Down
2 changes: 0 additions & 2 deletions contracts/core/vault/interfaces/IVault.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
// author: @stevieraykatz
pragma solidity ^0.8.19;

import "../../../core/router/IRouter.sol";

abstract contract IVault {
/*////////////////////////////////////////////////
CUSTOM TYPES
Expand Down