Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

update SMS verification #3579

Merged
merged 10 commits into from
Nov 28, 2016
Merged

update SMS verification #3579

merged 10 commits into from
Nov 28, 2016

Conversation

derhuerst
Copy link
Contributor

This PR adapts the SMS verification modal to the changes in the server and adds a few more checks. Fixes #3514.

@chevdor Please check, I will keep the ropsten server running now.

22e1384 and c62c72d make the modal wait until the Puzzled event, posted by the server, has actually been mined. This prevents the follow-up confirm tx from failing (and therefore using up all gas).

As requested by @gavofyork, 6509252 makes the modal check if the code is valid using contract.confirm.call().

@derhuerst derhuerst added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Nov 23, 2016
@chevdor
Copy link

chevdor commented Nov 23, 2016

I cannot check right now but I will within 2h max.

@derhuerst
Copy link
Contributor Author

@jacogr @ngotchac

util/subscribe-to-event is a helper function that returns an event-based interface to listen for events.

Because the promise returned by Contract.prototype.subscribe resolves after the callback has been called for the first time, it is hard to unsubscribe if you already got what you want. You will have to store that you want to unsubscribe as soon as the promise resolves.

I think it now depends which interface is easier to consume. If you want many events, using the event-based interface is really nice. For one, it won't be worth the boilerplate.

@chevdor
Copy link

chevdor commented Nov 23, 2016

I merged jr-update-sms-verification and rebuilt.
I am still getting Failed to request a confirmation SMS: Failed to fetch

@derhuerst
Copy link
Contributor Author

derhuerst commented Nov 23, 2016

@chevdor Sorry, I forgot to mention that it only works on ropsten. For me and @jacogr, it just worked. Will ping you on Gitter with details.

@chevdor
Copy link

chevdor commented Nov 23, 2016

@derhuerst I also forgot to mention it I am also (now) on Ropsten. If it works for you both, I can propose you to connect with me and we check together or we leave it as it is, and I test again once the PRs is merged. Up to you.

@derhuerst
Copy link
Contributor Author

derhuerst commented Nov 23, 2016

Still to do:

@derhuerst derhuerst added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 23, 2016
@derhuerst derhuerst added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Nov 23, 2016
// along with Parity. If not, see <http://www.gnu.org/licenses/>.

export default (chain) => {
return chain === 'morden' || chain === 'ropsten' || chain === 'testnet';
Copy link
Contributor

Choose a reason for hiding this comment

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

? Redux has isTest, don't want to maintain it in yet another place, we just simplified and got it down to 1. (And had to make changes.)

In addition, we are really starting to clutter /utils now with all these single function files. There is no reason why things can't be grouped, e.g. had to move nullable to protypes once we added another one. (General grumble)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed as of 88cb1df.

// You should have received a copy of the GNU General Public License
// along with Parity. If not, see <http://www.gnu.org/licenses/>.

import EventEmitter from 'eventemitter3';
Copy link
Contributor

Choose a reason for hiding this comment

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

Would have really preferred a general solution that (a) doesn't add another way of doing things, (b) is available to everybody. But suppose it solves the immediate issue, so once we have a general solution in-place will have to come back.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling f1bc69a on jr-update-sms-verification into ** on master**.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 85.908% when pulling 88cb1df on jr-update-sms-verification into a7037f8 on master.

@jacogr jacogr added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 28, 2016
@jacogr jacogr merged commit 5f570ed into master Nov 28, 2016
@jacogr jacogr deleted the jr-update-sms-verification branch November 28, 2016 16:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants