Skip to content

Conversation

@bergauz
Copy link
Contributor

@bergauz bergauz commented Oct 28, 2019

Screenshot 2019-10-30 at 20 27 03

@bergauz bergauz requested review from jmank88 and r-gochain October 28, 2019 14:48

private _web3Callable$: BehaviorSubject<Web3> = new BehaviorSubject(null);
private _web3Payable$: BehaviorSubject<Web3> = new BehaviorSubject(null);
private _web3Metamask$: BehaviorSubject<Web3> = new BehaviorSubject(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Switching back to Metamask specific naming seems backwards. Why don't we keep these general, for example:

walletInstalled$:
walletConfigured$:
_web3Payable$:

Then if/when we switch or add support for others, none of this needs to be updated.
(The user messages can stay metamask specific until we actually add more)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jmank88 can you check new code, wallet and metamask is different, pk is also wallet

@jmank88
Copy link
Contributor

jmank88 commented Oct 29, 2019

Configuration cases to test:

  • MM
  • PK w/o MM
  • PK w/ MM
  • PK w/ MM misconfigured (different net id)

@bergauz
Copy link
Contributor Author

bergauz commented Oct 30, 2019

@jmank88 if MM available there's no option to enter PK

@bergauz bergauz marked this pull request as ready for review October 30, 2019 11:18
@jmank88 jmank88 mentioned this pull request Oct 31, 2019
7 tasks
@treeder
Copy link
Contributor

treeder commented Jan 2, 2020

What are we doing with this?

@bergauz
Copy link
Contributor Author

bergauz commented Jan 20, 2020

@treeder pushed phase 1 from #440 , have some questions, should user enter PK each time when sending transaction?

@treeder
Copy link
Contributor

treeder commented Jan 20, 2020

No, if they stay on the page, leave the pk in the field.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants