Changes needed for additional youtube transfer UI #62

2019-10-01T05:15:11+02:00

neb-b commented

2019-10-01 05:15:11 +02:00

(Migrated from github.com)

No description provided.

neb-b (Migrated from github.com) reviewed 2019-10-01 05:19:42 +02:00

					
				@ -3,2 +3,4 @@

				export const GENERATE_AUTH_TOKEN_STARTED = 'GENERATE_AUTH_TOKEN_STARTED';

				export const GENERATE_AUTH_TOKEN_SUCCESS = 'GENERATE_AUTH_TOKEN_SUCCESS';

				export const AUTHENTICATION_STARTED = 'AUTHENTICATION_STARTED';

				export const AUTHENTICATION_SUCCESS = 'AUTHENTICATION_SUCCESS';

neb-b (Migrated from github.com) commented

2019-10-01 05:19:42 +02:00

We were importing all of these from lbry-redux. Not sure why

We were importing all of these from `lbry-redux`. Not sure why

akinwale (Migrated from github.com) requested changes 2019-10-03 23:22:38 +02:00

akinwale (Migrated from github.com) left a comment

One issue with the doSetSync method. Everything else looks good.

One issue with the `doSetSync` method. Everything else looks good.

src/redux/actions/rewards.js

akinwale (Migrated from github.com) commented

2019-10-03 23:17:33 +02:00

While I understand the reasoning behind this,I feel it's a bit weird because it may be possible for the sdk to not have the updated balance after a second. Maybe we could also explore using messages like "your wallet balance will be updated shortly" or something similar. Not really a blocker though.

src/redux/actions/sync.js

akinwale (Migrated from github.com) commented

2019-10-03 23:20:16 +02:00

Should we rename this to something more descriptive? Right now, this calls /sync/set, and then sets the default account if the hashes are different, and finally unlocks the the wallet if it's locked. It is also possible for the account_unlock call to fail with an error. If that happens, should SET_SYNC_FAILED be dispatched?

Alternatively, we could have two methods. One for calling /sync/set and the another one calling /sync/set with the extra steps.

Should we rename this to something more descriptive? Right now, this calls `/sync/set`, and then sets the default account if the hashes are different, and finally unlocks the the wallet if it's locked. It is also possible for the `account_unlock` call to fail with an error. If that happens, should `SET_SYNC_FAILED` be dispatched? Alternatively, we could have two methods. One for calling `/sync/set` and the another one calling `/sync/set` with the extra steps.

neb-b (Migrated from github.com) reviewed 2019-10-04 06:44:23 +02:00

src/redux/actions/rewards.js

neb-b (Migrated from github.com) commented

2019-10-04 06:44:23 +02:00

Yeah, I agree it's not a very good solution. I think the better fix is to wait for the SDK to add balance updates through sockets.

neb-b (Migrated from github.com) reviewed 2019-10-04 06:46:02 +02:00

src/redux/actions/sync.js

neb-b (Migrated from github.com) commented

2019-10-04 06:46:02 +02:00

I think it makes it a lot nicer to work with if theres just one method. But I can add a parameter to the function that triggers the additional steps

neb-b (Migrated from github.com) reviewed 2019-10-04 09:01:12 +02:00

src/redux/actions/sync.js

neb-b (Migrated from github.com) commented

2019-10-04 09:01:12 +02:00

After looking at this again after your comment I realized it should have been pulled out. I put it in doGetSync with a flag that starts the additional steps. This shouldn't require any changes from you.

After looking at this again after your comment I realized it should have been pulled out. I put it in `doGetSync` with a flag that starts the additional steps. This shouldn't require any changes from you.

akinwale (Migrated from github.com) approved these changes 2019-10-04 15:08:46 +02:00

akinwale (Migrated from github.com) left a comment

Looks good.

Sign in to join this conversation.

No reviewers

akinwale

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: LBRYCommunity/lbryinc#62