Transferrable subgraph ownership #497

abarmat · 2021-08-31T02:02:26Z

Motivation

App developers create subgraphs to index blockchain data. Then, they want indexers to run their subgraphs in the decentralized network. To achieve that, they publish a Subgraph in the GNS that targets a Subgraph Deployment. Once they publish a subgraph, they work to attract curators that delegate signal to the Subgraph so that the app developer can properly incentivize indexers.

For many reasons, the app developer might want to transfer ownership of the subgraph to a different account, a valid use case, not possible with the current implementation.

Specification

https://forum.thegraph.com/t/subgraph-ownership-transfer/2589

davekaj

Done. I like it overall, you are on the right path

davekaj · 2021-08-31T20:10:49Z

contracts/discovery/GNS.sol

+            _newSubgraphDeploymentID != namePool.subgraphDeploymentID,
+            "GNS: Cannot publish a new version with the same subgraph deployment ID"
+        );
+        // TODO: review this. why can't we allow new versions on non-curated ones?


This was because the GNS used the Bancor Formula. So when we tried this upgrade, it would fail because we can't instantiate a Bancor curve with 0 tokens. We had curation enforce a minimum signal, but it didn't seem practical to have this as a requirement on GNS, since it would get the owner in a weird spot. It would need a minimum of Shares.

But we have an upgrade for the GNS to remove it from using the GNS. So, when that upgrade goes through, we can remove this requirement!

davekaj · 2021-08-31T20:20:07Z

contracts/discovery/GNS.sol

-        uint256 tokensWithTax = _chargeOwnerTax(tokens, _graphAccount, curationTaxPercentage);
+        // Upgrade is only callable by the owner, we asume then that msg.sender = owner
+        address subgraphOwner = msg.sender;
+        uint256 tokensWithTax = _chargeOwnerTax(


maybe we can rework the _changeOwnerTax() function. its implementation is awkward, as it tries to charge the owner the owner tax, but the curation tax eats into that. So we force the owner to pay slightly more, rather than the curators. its odd, I always felt like it could be better

I agree, I would make it part of a different PR :)

davekaj · 2021-08-31T20:23:44Z

contracts/discovery/GNS.sol

-    function mintNSignal(
-        address _graphAccount,
-        uint256 _subgraphNumber,
+    function mintSignal(


everything was named with the N in it, such as mintNSignal, to distingush the different of minting on the GNS and minting on Curation. To help with the Subgraph and the front end for integrations.

Removing the N is okay with me. Just thought I'd mention it in case we want to keep a slight difference somehow. but it should be okay, as they are different contracts

Yes, I tried to remove the N and V and make it all about subgraph and subgraph deployment, that is how I find we are calling the things in practice. In the Curation contract it is just plain mint()

davekaj · 2021-08-31T20:27:40Z

contracts/discovery/GNS.sol

+        namePool.reserveRatio = defaultReserveRatio;
+
+        emit SubgraphCreated(subgraphID, _subgraphDeploymentID, defaultReserveRatio);
+        emit SubgraphMetadataUpdated(subgraphID, _subgraphMetadata);


could we call the function updateSubgraphMetadata() instead? Since all it does is emit the event

Yes, I've just temporarily moved into a plain emit here to remove dependencies and see clearly what happens. I'll have as a note to re-use that function.

contracts/discovery/GNS.sol

davekaj · 2021-08-31T20:37:25Z

contracts/discovery/GNS.sol

        require(
            _nSignal <= curatorNSignal,
            "GNS: Curator cannot withdraw more nSignal than they have"
        );

        // Get tokens for name signal amount to burn
-        uint256 vSignal = nSignalToVSignal(_graphAccount, _subgraphNumber, _nSignal);
+        uint256 vSignal = nSignalToVSignal(_subgraphID, _nSignal);


maybe a bit odd we have nSignal and vSignal inside of the functions, but we have renamed events and functions.

i never liked the naming 😂 .maybe we can make it overall better and more understandable. no one in the community refers to nSignal or vSignal. its not a well understood concept. its just 'signal' to most people

Yes, we can think of renaming, it's a bit confusing, the tricky thing is to find good names for this

davekaj · 2021-08-31T21:52:42Z

contracts/discovery/GNSStorage.sol

+
+    // Every time an account creates a subgraph it increases a per-account sequence ID
+    // account => seqID
+    mapping(address => uint256) public nextAccountSeqID;


maybe accountSeqID or seqID will suffice, wdyt?

The reason I did next was to make it clear that the number stored there is not the last one used but the next one that will be used when you create a subgraph, do you think there is a better name?

Ah I see.

I think there is probably a better name. I took 10 mins to try to think of one but I didn't come up with one. We can think about it while the PR is open, if nothing comes up im happy to use this name

expectedAccountSeqID or similar?

subsequentAccountId?

davekaj · 2021-08-31T22:04:21Z

contracts/discovery/GNS.sol

     */
-    function deprecateSubgraph(address _graphAccount, uint256 _subgraphNumber)
+    function deprecateSubgraph(uint256 _subgraphID)


will we burn the NFT here?

I was originally thinking about burning the NFT but I think we could just keep it there and be useless. The reason for that is that if we want to burn I need to get it transferred to the contract. I see no harm in the user keeping the NFT.

Might be worth while to think through what the Front end experience will be like.

If we are displaying NFTs, or someone goes to OpenSea and sees subgraph NFTs, it might be confusing.

Also if a subgraph developer has like 15 subgraphs, 8 of them deprecated, they might need to sift through them, like on etherscan.

maybe we burn, maybe we dont. lets think about it

I decided to burn the token so it's not sitting there as something that someone can transfer but with no purpose to manage the subgraph.

No need to transfer the NFT to the GNS to burn it, we can use the internal _burn function because the GNS is the NFTRegistry.

function _burn(uint256 tokenId) internal virtual { address owner = ERC721Upgradeable.ownerOf(tokenId); // internal owner _beforeTokenTransfer(owner, address(0), tokenId); // Clear approvals _approve(address(0), tokenId); // Clear metadata (if any) if (bytes(_tokenURIs[tokenId]).length != 0) { delete _tokenURIs[tokenId]; } _holderTokens[owner].remove(tokenId); _tokenOwners.remove(tokenId); emit Transfer(owner, address(0), tokenId); }

davekaj · 2021-08-31T22:07:16Z

contracts/discovery/GNS.sol

-        namePool.disabled = true;
-
-        emit NameSignalDisabled(_graphAccount, _subgraphNumber, namePool.withdrawableGRT);
+    // TODO: review this function


Is this in reference to the fact we will have to handle V1 subgraphs, as well as the NFT subgraphs?

Seems complex. But doable

_getSubgraphPool will give you the storage for the v1 subgraphs or the v2 subgraphs depending when it was created. We can't migrate old curators to the new struct.

davekaj · 2021-08-31T22:15:17Z

contracts/discovery/GNSStorage.sol


-    // graphAccountID => subgraphNumber => NameCurationPool
-    mapping(address => mapping(uint256 => IGNS.NameCurationPool)) public nameSignals;
+    // TODO: use it whenever an "old" subgraph NFT was claimed to maintain compatibility


I think this strategy will work for upgrading permissionlessly.

We could also consider that we just mint the NFTs on migration for our users. We could do it automatically, like prefill this data, or we could make it more permissionless by making it a function anyone can call.

The benefits are that we do not have to explain to the users how to do the migrate. I think a bit about the LEND --> AAVE migration. It is something that had to be explained, communicated, and had some split for a while on the two tokens. And then, even governance votes to close the migration contract, many months in the future.

But just an idea. it can be discussed

Yes, I was thinking on making the claim function for the NFT public. Then to get it called for all existing subgraphs after an upgrade. That would initialize the required structures so anyone can call the contract functions with the new primary key.

abarmat · 2021-09-01T20:10:36Z

contracts/discovery/GNS.sol

+    }
+
+    // TODO: claim an NFT and send it to the owner if not already done
+    function claimLegacy(address _graphAccount, uint256 _subgraphNumber) external {


Rename to migrateLegacy

abarmat · 2021-10-06T22:57:55Z

Rebased, squashed and clean up

codecov · 2021-10-06T23:09:32Z

Codecov Report

Merging #497 (2cf753f) into dev (6962037) will decrease coverage by 1.31%.
The diff coverage is 83.70%.

❗ Current head 2cf753f differs from pull request most recent head 1c15e48. Consider uploading reports for the commit 1c15e48 to get more accurate results

@@            Coverage Diff             @@
##              dev     #497      +/-   ##
==========================================
- Coverage   91.91%   90.60%   -1.32%     
==========================================
  Files          33       35       +2     
  Lines        1658     1703      +45     
  Branches      282      282              
==========================================
+ Hits         1524     1543      +19     
- Misses        134      160      +26

Flag	Coverage Δ
unittests	`90.60% <83.70%> (-1.32%)`	⬇️

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

Impacted Files	Coverage Δ
contracts/base/SubgraphNFTDescriptor.sol	`0.00% <0.00%> (ø)`
contracts/discovery/GNS.sol	`87.11% <84.12%> (-12.15%)`	⬇️
contracts/base/SubgraphNFT.sol	`100.00% <100.00%> (ø)`
...ontracts/discovery/erc1056/EthereumDIDRegistry.sol	`0.00% <0.00%> (-7.02%)`	⬇️
contracts/curation/Curation.sol	`100.00% <0.00%> (ø)`
contracts/curation/GraphCurationToken.sol	`100.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6962037...1c15e48. Read the comment docs.

- make the subgraph to use a single primary key - major refactor of data structures - refactor variable names and add migrate legacy subgraph - get subgraph data depending if legacy or new subgraph - improve validations - refactor all subgraph checks - update tests - NFT burn and mint

…r to operate

…ed signal

Audit: [N03] Typographical and grammatical errors

Audit: [N05] Unnecessary use of msg.sender

Audit: [N01] Lack of input validation

Audit: [M01] Semantic overloading of approvals for access control

Audit: [X01] Subgraph deployment not updated when no signal

Audit: [X02] Update event name

davekaj reviewed Aug 31, 2021

View reviewed changes

abarmat commented Sep 1, 2021

View reviewed changes

abarmat marked this pull request as ready for review September 22, 2021 12:51

abarmat added the GIP label Sep 22, 2021

abarmat added the in progress label Oct 5, 2021

abarmat force-pushed the ariel/gns-ownership branch from f78723b to 9ef9062 Compare October 6, 2021 22:57

abarmat force-pushed the ariel/gns-ownership branch from 9ef9062 to f1f6624 Compare October 6, 2021 22:58

abarmat added 2 commits October 14, 2021 15:06

feat: add NFT descriptor for a subgraph

396d06b

abarmat force-pushed the ariel/gns-ownership branch from 4448e94 to 396d06b Compare October 14, 2021 18:49

abarmat mentioned this pull request Oct 14, 2021

Updateable reserveRatio for GNS #515

Closed

abarmat added 14 commits October 30, 2021 00:42

fix: correct function doc

c152f48

feat: update subgraph authorization policy to only allow the NFT owne…

c66614a

…r to operate

fix: correct typos

6ec4065

refactor: use curator variable instead of msg.sender

66d6e5e

fix: validate nSignalToTokens reverting when subgraph is not published

321c29b

fix: update the target deployment even if the Subgraph has never mint…

3482568

…ed signal

refactor: use SubgraphPublished event

483add1

Merge pull request #520 from graphprotocol/audit/gns-ownership-n03

237a26d

Audit: [N03] Typographical and grammatical errors

Merge pull request #521 from graphprotocol/audit/gns-ownership-n05

78b1e39

Audit: [N05] Unnecessary use of msg.sender

Merge pull request #522 from graphprotocol/audit/gns-ownership-n01

4d028de

Audit: [N01] Lack of input validation

Merge pull request #518 from graphprotocol/audit/gns-ownership-l01

2cf753f

Audit: [N01] Lack of input validation

Merge pull request #519 from graphprotocol/audit/gns-ownership-m01

6a86c16

Audit: [M01] Semantic overloading of approvals for access control

Merge pull request #523 from graphprotocol/audit/gns-ownership-x01

af5f8a5

Audit: [X01] Subgraph deployment not updated when no signal

Merge pull request #524 from graphprotocol/audit/gns-ownership-x02

f31b65f

Audit: [X02] Update event name

abarmat added audited and removed in progress labels Dec 1, 2021

chore: update deployment script to initialize the new GNS

0719e54

abarmat added 2 commits December 1, 2021 13:58

docs: add audit report for transferrable subgraph

1c15e48

Merge branch 'dev' into ariel/gns-ownership

a3aa132

abarmat merged commit acc3993 into dev Dec 1, 2021

abarmat mentioned this pull request Dec 13, 2021

Refactor Subgraph NFT using composition #527

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transferrable subgraph ownership #497

Transferrable subgraph ownership #497

abarmat commented Aug 31, 2021 •

edited

Loading

davekaj left a comment

davekaj Aug 31, 2021

abarmat Aug 31, 2021

davekaj Aug 31, 2021

abarmat Aug 31, 2021

davekaj Aug 31, 2021

davekaj Aug 31, 2021

abarmat Aug 31, 2021

davekaj Aug 31, 2021

abarmat Aug 31, 2021

davekaj Aug 31, 2021

abarmat Aug 31, 2021 •

edited

Loading

davekaj Aug 31, 2021

abarmat Aug 31, 2021

davekaj Aug 31, 2021

chriswessels Sep 30, 2021

jona Oct 15, 2021

davekaj Aug 31, 2021

abarmat Aug 31, 2021

davekaj Aug 31, 2021

abarmat Oct 15, 2021

davekaj Aug 31, 2021

abarmat Aug 31, 2021

davekaj Aug 31, 2021

abarmat Aug 31, 2021

davekaj Aug 31, 2021

abarmat Sep 1, 2021

abarmat commented Oct 6, 2021

codecov bot commented Oct 6, 2021 •

edited

Loading

Transferrable subgraph ownership #497

Transferrable subgraph ownership #497

Conversation

abarmat commented Aug 31, 2021 • edited Loading

Motivation

Specification

davekaj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abarmat Aug 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abarmat commented Oct 6, 2021

codecov bot commented Oct 6, 2021 • edited Loading

Codecov Report

abarmat commented Aug 31, 2021 •

edited

Loading

abarmat Aug 31, 2021 •

edited

Loading

codecov bot commented Oct 6, 2021 •

edited

Loading