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.

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

feat: implement user sign in and sign up #36

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

pingoo33
Copy link

No description provided.

@pingoo33 pingoo33 changed the title feat: implement user sign in feat: implement user sign in and sign up Mar 14, 2020
@pingoo33 pingoo33 force-pushed the ghostApi branch 3 times, most recently from a8012dd to 7b9cee7 Compare March 15, 2020 06:29
Copy link
Collaborator

@ttkmw ttkmw left a comment

Choose a reason for hiding this comment

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

전반적으로 굿굿!!
코멘트 확인해주삼

그리고 서버 키고 로그인되는거 확인해봤다는거지??

src/api/auth/auth.api.spec.ts Outdated Show resolved Hide resolved
src/api/auth/auth.api.spec.ts Outdated Show resolved Hide resolved
src/api/auth/auth.api.ts Outdated Show resolved Hide resolved
@@ -4,11 +4,12 @@ import {Ghost} from '@/types';

@Service()
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 부분엔 어떤 수정이 있는 건지.. 실수로 추가된거라면 빼주세요~!

src/api/wallet/wallet.service.impl.ts Outdated Show resolved Hide resolved
}

public async getPublicAddress(): Promise<string> {
return await window.ethereum.enable()[0].address;
Copy link
Collaborator

Choose a reason for hiding this comment

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

하드코딩을 빼면 더 좋을 것 같네요.
0이 어떤 의미인가요? 1, 2는 어떤의미이죠?

Copy link
Author

Choose a reason for hiding this comment

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

metamask의 default account를 가져오는 것입니다.
실제로 defaultAccount라는 속성이 있지만 안정성 문제를 이유로 0번째 account에 접근하는 방식을 사용합니다.

src/api/wallet/wallet.service.impl.ts Outdated Show resolved Hide resolved
src/api/wallet/wallet.service.impl.ts Outdated Show resolved Hide resolved
@@ -0,0 +1,5 @@
export interface WalletService {
hasWallet(): boolean;
Copy link
Collaborator

Choose a reason for hiding this comment

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

hasWallet()은 결국 publicAddress를 받기 위한 사전작업 아닌가요?
hasWallet()까지 드러내진 않아도 될 것 같은데...
만약 wallet을 반드시 가져야한다면, confirmWallet하고 리턴으로 wallet을 전달해주던가, getWallet으로 wallet을 전달해주는 게 낫지 않을까 싶습니다.

Copy link
Author

Choose a reason for hiding this comment

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

wallet이라는 속성을 manage하는 것이 아닌, wallet에 연결되어 있는가를 확인하는 함수입니다.
interface에 추가 시킨 이유는 이 interface를 implements하는 모든 wallet service가 반드시 있어야 하는 기능이라고 생각했습니다.

src/store/modules/user.ts Outdated Show resolved Hide resolved
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.

2 participants