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

[ETHEREUM-CONTRACTS] Disable flownfts #1991

Merged
merged 25 commits into from
Aug 5, 2024
Merged

[ETHEREUM-CONTRACTS] Disable flownfts #1991

merged 25 commits into from
Aug 5, 2024

Conversation

d10r
Copy link
Collaborator

@d10r d10r commented Jul 17, 2024

See #1993

deploy-framework.js won't deploy FlowNFT contracts anymore. But it will preserve existing contract addresses if set.

Note that only enough code to keep the CI pipeline green was removed, there's still a lot of FlowNFT related code left.
As is in this PR, it would be relatively easy to "bring it back" later.
In order to fully purge, a lot more code locations need to be touched.

piggy-backed:

  • include previous canonical superTokenLogic in list of logic addresses to be updated from

Copy link

github-actions bot commented Jul 17, 2024

Changelog Reminder

Reminder to update the CHANGELOG.md for any of the modified packages in this PR.

  • CHANGELOG.md modified
  • Double check before merge

@d10r d10r force-pushed the disable-flownfts branch 2 times, most recently from 52a1439 to ee5796e Compare July 18, 2024 17:08
@d10r d10r linked an issue Jul 22, 2024 that may be closed by this pull request
@d10r d10r marked this pull request as ready for review July 22, 2024 13:33
@d10r d10r requested a review from hellwolf as a code owner July 22, 2024 13:33
Copy link
Contributor

@hellwolf hellwolf left a comment

Choose a reason for hiding this comment

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

  • remove eNFT completely from code base
  • Remove all eNFT references in ISuperToken
  • Make sure deployment scripts no longer dependent on the eNFT interfaces.

@hellwolf
Copy link
Contributor

to be released as v1.11.0

@d10r d10r requested a review from kasparkallas as a code owner August 1, 2024 10:42
getOrInitAccount(event.params.from, event.block);
}

export function handleMetadataUpdate(event: MetadataUpdate): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

Only the FlowNFT handling of the event disappears, right, not MetadataUpdate altogether?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

didn't realize this is FlowNFT specific too. So, yes, I guess more pruning...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed now

packages/subgraph/schema.graphql Show resolved Hide resolved
@kasparkallas kasparkallas requested a review from a team as a code owner August 1, 2024 12:37
@d10r
Copy link
Collaborator Author

d10r commented Aug 1, 2024

removal of an event / attributes (?) will break getEvents() in sdk-core.
Thus this order of events is important:

  • create new sdk-core package and update products
  • switch canonical subgraph to new schema

@d10r
Copy link
Collaborator Author

d10r commented Aug 1, 2024

reason for docker-compose -> docker compose: actions/runner-images#9692

Copy link

codecov bot commented Aug 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.55%. Comparing base (5106c44) to head (d9743c3).

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #1991      +/-   ##
==========================================
- Coverage   88.63%   88.55%   -0.08%     
==========================================
  Files         119      111       -8     
  Lines        7388     6945     -443     
  Branches      981      933      -48     
==========================================
- Hits         6548     6150     -398     
+ Misses        838      793      -45     
  Partials        2        2              
Flag Coverage Δ
ethereum-contracts 94.76% <ø> (+0.53%) ⬆️
sdk-core 88.55% <100.00%> (-0.08%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hellwolf
Copy link
Contributor

hellwolf commented Aug 5, 2024

$ git grep -i flownft packages/ | wc -l
97

$ git grep -i flownft packages/ | grep -Fv CHANGELOG.md | head -n10
packages/ethereum-contracts/contracts/mocks/SuperTokenFactoryMock.t.sol:import { SuperTokenFactoryBase, IConstantOutflowNFT, IConstantInflowNFT } from "../superfluid/SuperTokenFactory.sol";
packages/ethereum-contracts/contracts/mocks/SuperTokenFactoryMock.t.sol:        IConstantOutflowNFT constantOutflowNFT,
packages/ethereum-contracts/contracts/mocks/SuperTokenFactoryMock.t.sol:        IConstantInflowNFT constantInflowNFT,
packages/ethereum-contracts/contracts/mocks/SuperTokenFactoryMock.t.sol:        SuperTokenFactoryBase(host, superTokenLogic, constantOutflowNFT, constantInflowNFT, poolAdminNFT, poolMemberNFT)
packages/ethereum-contracts/contracts/mocks/SuperTokenFactoryMock.t.sol:        IConstantOutflowNFT constantOutflowNFT,
packages/ethereum-contracts/contracts/mocks/SuperTokenFactoryMock.t.sol:        IConstantInflowNFT constantInflowNFT,
packages/ethereum-contracts/contracts/mocks/SuperTokenFactoryMock.t.sol:        SuperTokenFactoryBase(host, superTokenLogic, constantOutflowNFT, constantInflowNFT, poolAdminNFT, poolMemberNFT)
packages/ethereum-contracts/contracts/mocks/SuperTokenFactoryMock.t.sol:        IConstantOutflowNFT constantOutflowNFT,
packages/ethereum-contracts/contracts/mocks/SuperTokenFactoryMock.t.sol:        IConstantInflowNFT constantInflowNFT,
packages/ethereum-contracts/contracts/mocks/SuperTokenFactoryMock.t.sol:        SuperTokenFactoryBase(host, superTokenLogic, constantOutflowNFT, constantInflowNFT, poolAdminNFT, poolMemberNFT)

$...

What are these remnants?

@d10r
Copy link
Collaborator Author

d10r commented Aug 5, 2024

The currently deployed Supertoken.sol has this in updateCode():

        // @note This is another check to ensure that when updating to a new SuperToken logic contract
        // that we have passed the correct NFT proxy contracts in the construction of the new SuperToken
        // logic contract
        if (
            CONSTANT_OUTFLOW_NFT !=
            SuperToken(newAddress).CONSTANT_OUTFLOW_NFT() ||
            CONSTANT_INFLOW_NFT !=
            SuperToken(newAddress).CONSTANT_INFLOW_NFT()
        ) {
            revert SUPER_TOKEN_NFT_PROXY_ADDRESS_CHANGED();
        }

So we can't set the address to zero in this upgrade, meaning we can't fully prune in 1 iteration.
This PR removes this check, so in a next iteration we could complete the pruning.

Also I already removed CONSTANT_OUTFLOW_NFT() and CONSTANT_OUTFLOW_NFT() from ISuperToken.

Copy link
Contributor

@hellwolf hellwolf left a comment

Choose a reason for hiding this comment

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

:) great!

@hellwolf hellwolf added this pull request to the merge queue Aug 5, 2024
@d10r d10r self-assigned this Aug 5, 2024
@d10r d10r linked an issue Aug 5, 2024 that may be closed by this pull request
2 tasks
@d10r d10r removed this pull request from the merge queue due to a manual request Aug 5, 2024
@d10r d10r enabled auto-merge August 5, 2024 14:32
@d10r d10r added this pull request to the merge queue Aug 5, 2024
Merged via the queue into dev with commit 69c5856 Aug 5, 2024
19 of 20 checks passed
@d10r d10r deleted the disable-flownfts branch August 5, 2024 15:46
Copy link

github-actions bot commented Aug 5, 2024

XKCD Comic Relif

Link: https://xkcd.com/1991
https://xkcd.com/1991

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.

[ETHEREUM-CONTRACTS] [METADATA] [SDK-CORE] remove flowNFT code [ETHEREUM-CONTRACTS] Disable FlowNFT hooks
3 participants