kredits/kredits-web
9
0
Fork 0
Code Issues 21 Pull Requests Actions Releases 19 Activity

Notify to change selected network #143

Closed
haythemsellami wants to merge 1 commits from detect-selected-network into master
pull from: detect-selected-network
merge into: :master
Conversation 5 Commits 1 Files Changed 3
+53 -13
haythemsellami commented 2019-07-22 13:43:25 +00:00 (Migrated from github.com)
Copy Link
Copy Source

This PR related to #82 .
It check the selected network and the network Kredits contract deployed at and notify the user with a simple alert in case both networks are not the same without running kredits.setup().

This PR related to #82 . It check the selected network and the network Kredits contract deployed at and notify the user with a simple alert in case both networks are not the same without running `kredits.setup()`.
haythemsellami commented 2019-07-22 13:44:01 +00:00 (Migrated from github.com)
Copy Link
Copy Source

I'm not really sure if this is the right way to do it with the existing project architecture...

I'm not really sure if this is the right way to do it with the existing project architecture...
raucao (Migrated from github.com) requested changes 2019-07-22 14:57:42 +00:00
raucao (Migrated from github.com) left a comment
Copy Link
Copy Source

This looks/sounds to me like it doesn't work with the local development network, or does it?

This looks/sounds to me like it doesn't work with the local development network, or does it?
app/routes/application.js
@@ -18,0 +19,4 @@
kreditsNetworkName = "Morden Test";
break;
case '3':
kreditsNetworkName = "Ropsten Test";
raucao (Migrated from github.com) commented 2019-07-22 14:56:13 +00:00
Copy Link
Copy Source

This belongs in a separate function or config. But from what I remember I'm pretty sure you can get network names from the ether.js library/API anyway, so there's no need to re-implement our own version of this.

This belongs in a separate function or config. But from what I remember I'm pretty sure you can get network names from the `ether.js` library/API anyway, so there's no need to re-implement our own version of this.
app/services/kredits.js
@@ -137,2 +137,4 @@
},
getKreditsDeployedNetwork() {
return Kredits.availableNetworks();
raucao (Migrated from github.com) commented 2019-07-22 14:55:18 +00:00
Copy Link
Copy Source

I don't understand how one can get a single network from a list of networks without filtering here.

I don't understand how one can get a single network from a list of networks without filtering here.
haythemsellami (Migrated from github.com) reviewed 2019-07-25 12:12:06 +00:00
app/services/kredits.js
@@ -137,2 +137,4 @@
},
getKreditsDeployedNetwork() {
return Kredits.availableNetworks();
haythemsellami (Migrated from github.com) commented 2019-07-25 12:12:06 +00:00
Copy Link
Copy Source

mm I thought that there will be only the network where kredits is deployed at (etiher rinkeby, ropsten, mainnet...), or no ?

mm I thought that there will be only the network where kredits is deployed at (etiher rinkeby, ropsten, mainnet...), or no ?
raucao (Migrated from github.com) reviewed 2019-07-25 12:42:46 +00:00
app/services/kredits.js
@@ -137,2 +137,4 @@
},
getKreditsDeployedNetwork() {
return Kredits.availableNetworks();
raucao (Migrated from github.com) commented 2019-07-25 12:42:45 +00:00
Copy Link
Copy Source

We run a local network for development, making it at least two. But also, there could be more networks in the future, hence the plural in availableNetworks() (as opposed to getNetwork() or similar).

We talked about this in the call yesterday and @bumi had some good ideas. Would be good to write them down in either the issue or PR.

@haythem96 By the way, it would be super useful if you could join the weekly call from time to time to talk things through with other people and also stay up to date with the rest of the project.

We run a local network for development, making it at least two. But also, there could be more networks in the future, hence the plural in `availableNetworks()` (as opposed to `getNetwork()` or similar). We talked about this in the call yesterday and @bumi had some good ideas. Would be good to write them down in either the issue or PR. @haythem96 By the way, it would be super useful if you could join the weekly call from time to time to talk things through with other people and also stay up to date with the rest of the project.
haythemsellami (Migrated from github.com) reviewed 2019-07-25 13:07:22 +00:00
app/services/kredits.js
@@ -137,2 +137,4 @@
},
getKreditsDeployedNetwork() {
return Kredits.availableNetworks();
haythemsellami (Migrated from github.com) commented 2019-07-25 13:07:22 +00:00
Copy Link
Copy Source

yeah really sorry for missing the last calls... was trying to join but I miss for some reasons... I will be available on the next ones.

yeah really sorry for missing the last calls... was trying to join but I miss for some reasons... I will be available on the next ones.

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No Reviewers
raucao
haythemsellami
Labels
Clear labels
bug
Something is not working
 dev environment
Config, builds, CI, deployment, etc.
 docs
Documentation
 duplicate
This issue or pull request already exists
 enhancement
Improving existing functionality
 feature
New functionality
 idea
Something to consider
 invalid
Not a bug
 kredits-1
Small contribution
 kredits-2
Medium contribution
 kredits-3
Large contribution
 question
Looking for an answer
release
major
release
minor
release
patch
security
All your base are belong to us
 ui/ux
User interface, process design, etc.
 wontfix
This won't be fixed
No labels
Milestone
No items
No Milestone
Assignees
Clear assignees
bkero (bkero) bumi (bumi) fsmanuel (fsmanuel) galfert (galfert) greg (Greg) haythem (haythem) hueso (hueso) raucao (Râu Cao) slvrbckt (Nick Jennings)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: kredits/kredits-web#143