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.

Sign up for GitHub

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

Jump to bottom

Docs/ard upstream node #4243

Merged
merged 1 commit into from
May 16, 2018
Merged

Docs/ard upstream node #4243

merged 1 commit into from
May 16, 2018

Conversation

oskarth
Copy link
Contributor

@oskarth oskarth commented May 14, 2018 •
edited
Loading

ARD for upstream/LES/ULC stuff, mostly capturing consensus.

In status-react repo as it already has ARD structure and it impacts end users in terms of options etc.

http://thinkrelevance.com/blog/2011/11/15/documenting-architecture-decisions

Context Architecture for agile projects has to be described and defined differently. Not all decisions will be made at once, nor will all of them be done whe...

@oskarth oskarth closed this May 14, 2018
@oskarth oskarth reopened this May 14, 2018
@rasom
Copy link
Contributor

rasom commented May 14, 2018 •
edited by status-github-bot bot
Loading

apk uploaded to https://i.diawi.com/M27c2X for branch PR-4243

@rasom
Copy link
Contributor

rasom commented May 14, 2018 •
edited by status-github-bot bot
Loading

ipa uploaded to https://i.diawi.com/fP3UAz for branch PR-4243

adambabik
adambabik reviewed May 14, 2018
View reviewed changes
doc/decisions/0003-geth-node.md Outdated
promising route to take.

However, since ULC isn't ready yet, this means we are relying on Infura. This is
a bad idea from a bad idea from a decentralization and security point of view.
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo.

adambabik
adambabik reviewed May 14, 2018
View reviewed changes
doc/decisions/0003-geth-node.md
From a decentralization point of view we need the latter before we can release
the beta.

2. ULC MVP. Additionally, as soon as beta is released, assess and start to
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I know, ULC requires trusted peers to operate. How is it different from the upstream node in terms of decentralization and security?

Decentralization aspect can be explained that it's possible to connect to more than one node in contrast to the upstream node.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.

@themue @JekaMas @b00ris @arnetheduck do you want to elaborate on what your point of view is here?

Copy link

Choose a reason for hiding this comment

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

Trust is done by config so far, it has to be done explicitly. So my greatest problem with it is the UX. A distributed directory with a kind of trust rating might help. But that's something for long term. Providing own nodes hardcoded as trusted nodes could act as jump start.

@oskarth oskarth requested a review from themue May 14, 2018 09:16
@oskarth
Copy link
Contributor Author

oskarth commented May 15, 2018

@adambabik @themue Addressed your comments. Please review and approve if you are happy with this

Also added consequence "disable LES for beta"

@oskarth oskarth requested a review from goranjovic May 15, 2018 02:23
@rasom
Copy link
Contributor

rasom commented May 15, 2018 •
edited by status-github-bot bot
Loading

apk uploaded to https://i.diawi.com/k5yDkt for branch PR-4243

@rasom
Copy link
Contributor

rasom commented May 15, 2018 •
edited by status-github-bot bot
Loading

ipa uploaded to https://i.diawi.com/4PeuSu for branch PR-4243

@statustestbot
Copy link

93% of end-end tests have passed

Total executed tests: 15
Failed tests: 1
Passed tests: 14

Failed tests (1)

Click to expand
1. test_browse_link_entering_url_in_dapp_view

Looking for ChatNameText
ChatNameText is Browser

E AssertionError: assert False + where False = ('Википедия') + where = 'Browser'.startswith + where 'Browser' = .text + where = .chat_name_text

Device sessions:

Passed tests (14)

Click to expand
1. test_one_to_one_chat_messages
Device sessions:

2. test_send_transaction_from_daap
Device sessions:

3. test_send_eth_from_wallet_sign_now
Device sessions:

4. test_send_eth_to_request_from_wallet
Device sessions:

5. test_contact_profile_view
Device sessions:

6. test_public_chat
Device sessions:

7. test_network_switch
Device sessions:

8. test_transaction_send_command_one_to_one_chat
Device sessions:

9. test_transaction_send_command_wrong_password
Device sessions:

10. test_send_eth_to_request_in_one_to_one_chat
Device sessions:

11. test_send_stt_from_wallet_via_enter_recipient_address
Device sessions:

12. test_send_eth_to_request_in_group_chat
Device sessions:

13. test_transaction_send_command_group_chat
Device sessions:

14. test_group_chat_messages_and_delete_chat
Device sessions:

themue
themue approved these changes May 15, 2018
View reviewed changes
Copy link

@themue themue left a comment

Choose a reason for hiding this comment

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

LGTM. two comments.

doc/decisions/0003-geth-node.md Outdated
## Context

On the path to releasing a usable beta we've faced a variety of performance
issues (https://github.com/status-im/ideas/issues/55,
Copy link

Choose a reason for hiding this comment

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

Just nitpicking, can you create a list for better readability?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

doc/decisions/0003-geth-node.md Outdated
established by config (list of trusted peers).

Initially we could hardcode these nodes, with option to add your own. Later on
we can have some form of distributed directory with a rating system.
Copy link

Choose a reason for hiding this comment

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

Add a note that this also depends from adding ULC to go-ethereum and other implementations, not only our patched branch. As long as we're the only ULC supporters we'll have to run the trusted nodes on our own.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@asemiankevich
Copy link
Contributor

@oskarth i believe this could pass to test queue and be merged )

@rasom
Copy link
Contributor

rasom commented May 16, 2018 •
edited by status-github-bot bot
Loading

apk uploaded to https://i.diawi.com/y2rEvR for branch PR-4243

@rasom
Copy link
Contributor

rasom commented May 16, 2018 •
edited by status-github-bot bot
Loading

ipa uploaded to https://i.diawi.com/wfymYV for branch PR-4243

@oskarth oskarth merged commit aa108cd into develop May 16, 2018
@rasom rasom deleted the docs/ard-upstream-node branch May 19, 2018 06:54
@StatusWrike StatusWrike mentioned this pull request Nov 9, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t1MZXxBn6pAAtTqKTy5UotdeMqBw2CfGrUF t1MZXxBn6pAAtTqKTy5UotdeMqBw2CfGrUF t1MZXxBn6pAAtTqKTy5UotdeMqBw2CfGrUF approved these changes

@themue themue themue approved these changes

@goranjovic goranjovic goranjovic approved these changes

@adambabik adambabik adambabik approved these changes

@arnetheduck arnetheduck Awaiting requested review from arnetheduck

Assignees

@oskarth oskarth

Labels
None yet
Projects
No open projects
Legacy QA Automation Project
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@oskarth @rasom @statustestbot @asemiankevich @adambabik @goranjovic @themue @t1MZXxBn6pAAtTqKTy5UotdeMqBw2CfGrUF