-
Notifications
You must be signed in to change notification settings - Fork 8
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
Change Govern subgraph to work with GovernQueue only + Add a task to … #266
Conversation
…deploy GovernQueue
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov Report
@@ Coverage Diff @@
## main #266 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 3 3
Lines 54 56 +2
Branches 12 13 +1
=========================================
+ Hits 54 56 +2
Continue to review full report at Codecov.
|
packages/react-app/src/networks.ts
Outdated
// questsSubgraph: 'https://api.thegraph.com/subgraphs/name/corantin/quests-subgraph', | ||
questsSubgraph: 'https://api.thegraph.com/subgraphs/name/corantin/quests-subgraph-staging', | ||
// governSubgraph: 'https://api.thegraph.com/subgraphs/name/corantin/govern-1hive-rinkeby', | ||
governSubgraph: 'https://api.thegraph.com/subgraphs/name/corantin/govern-1hive-rinkeby-staging', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could put it array in the future to easily change beetween subgraphs dependen of network, wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{ | ||
"network": "rinkeby", | ||
"dataSources": { | ||
"GovernRegistry": [ | ||
{ | ||
"name": "GovernRegistry", | ||
"address": "0x7a044ff5148220866B9c21E9C6537C7c960eed40", | ||
"startBlock": 9932541 | ||
} | ||
] | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you removed that? btw its look almost exactly what i did in quest subgraph, if i knew here it already was there, i could just copy and paste. You ran in some problems to change that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rinkeby-staging should be the same as rinkeby in my opinion so only one is needed
The only think that should change is the deploy target (corantin/govern-subgraph-rinkeby-staging)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And further more, I dont need Registry I am now only listening on one GovernQueue. Maybe we might want support multiple in the future but right now there is no good reason to drag a GovernRegistry
Oops was supposed to import from other file not declare it haha
…On Thu., May 12, 2022, 4:31 p.m. Felipe Novaes F Rocha, < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In packages/hardhat/hardhat.config.ts
<#266 (comment)>:
> +function updateContractResult(
+ network: Network,
+ arg1: string,
+ arg2: { address: string; abi: import("hardhat-deploy/dist/types").ABI }
+) {
+ throw new Error("Function not implemented.");
+}
Its look imccomplete.
—
Reply to this email directly, view it on GitHub
<#266 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADJ7IORRGOJ5PTKLTHCOX6LVJVTCZANCNFSM5VWJYYCA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Ohh this is a thing lmao
…On Thu., May 12, 2022, 4:36 p.m. Felipe Novaes F Rocha, < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In packages/hardhat/hardhat.config.ts
<#266 (comment)>:
> + defaultConfig.CreateQuestDeposit.xdai.amount,
+ types.float
+ )
+ .setAction(async (args, hre) => {
+ const deployResult = await deployQuestFactory(hre, args);
+ console.log(
+ "Deployed quest factory (" + hre.network.name + "):",
+ deployResult.address
+ );
+ updateContractResult(hre.network, "QuestFactory", {
+ address: deployResult.address,
+ abi: deployResult.abi,
+ });
+ });
+
+task("newQuestFactory:rinkeby")
[image: image]
<https://user-images.githubusercontent.com/691510/168163457-cced9c62-af85-48ec-bbdd-295599d3041a.png>
Maybe you want setDescription for the others task too?
—
Reply to this email directly, view it on GitHub
<#266 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADJ7IOQVBV24RI2ONSJQKNDVJVTT7ANCNFSM5VWJYYCA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
We always need one or the other and the config is already driven by network
…On Thu., May 12, 2022, 4:46 p.m. Felipe Novaes F Rocha, < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In packages/react-app/src/networks.ts
<#266 (comment)>:
> + // questsSubgraph: 'https://api.thegraph.com/subgraphs/name/corantin/quests-subgraph',
+ questsSubgraph: 'https://api.thegraph.com/subgraphs/name/corantin/quests-subgraph-staging',
+ // governSubgraph: 'https://api.thegraph.com/subgraphs/name/corantin/govern-1hive-rinkeby',
+ governSubgraph: 'https://api.thegraph.com/subgraphs/name/corantin/govern-1hive-rinkeby-staging',
Maybe we could put it array in the future to easily change beetween
subgraphs dependen of network, wdyt?
—
Reply to this email directly, view it on GitHub
<#266 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADJ7IOVFBQFAG6F2A6ZHKJDVJVUZHANCNFSM5VWJYYCA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
I just dont need Govern registry right now. In the future, when we will
have a registry then we will probably litsten to it but anyway right now
the webapp does not support multiple GovernQueue (this is what the Registry
allow us to do)
…On Thu., May 12, 2022, 4:54 p.m. Felipe Novaes F Rocha, < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In packages/subgraphs/govern-subgraph/manifest/data/rinkeby-staging.json
<#266 (comment)>:
> -{
- "network": "rinkeby",
- "dataSources": {
- "GovernRegistry": [
- {
- "name": "GovernRegistry",
- "address": "0x7a044ff5148220866B9c21E9C6537C7c960eed40",
- "startBlock": 9932541
- }
- ]
- }
-}
Why you removed that? btw its look almost exactly what i did in quest
subgraph, if i knew here it already was there, i could just copy and paste.
You ran in some problems to change that?
—
Reply to this email directly, view it on GitHub
<#266 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADJ7IOTJIYE4ZTNA4LEXTT3VJVVYPANCNFSM5VWJYYCA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
…oy Govern and Quests subgraph
…ardhat-contracts file
@@ -124,6 +124,7 @@ | |||
"start": "cross-env HTTPS=true npm run sync-assets && react-app-rewired start", | |||
"start:local": "cross-env REACT_APP_CHAIN_ID=1337 yarn start", | |||
"start:rinkeby": "cross-env REACT_APP_CHAIN_ID=4 yarn start", | |||
"start:rinkeby-staging": "cross-env REACT_APP_CHAIN_ID=4 REACT_APP_STAGING=true yarn start", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To who want test it in vercel staging need run that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope vercel always run yarn start and rely on defaultChain and staging variables to decide wich network and environment
useEffect( | ||
() => () => { | ||
mounted = false; | ||
}, | ||
[], | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mayeb we can open PR to create some hook for it, i saw we have multiples those on the code base.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking the same but wasnt able to have something cleaner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work!
…deploy GovernQueue