Skip to content
This repository was archived by the owner on Aug 25, 2023. It is now read-only.

Feature: Add current user blockchain address request #252

Merged
merged 8 commits into from
Jul 19, 2019
Merged

Feature: Add current user blockchain address request #252

merged 8 commits into from
Jul 19, 2019

Conversation

MalcolmnDEV
Copy link
Contributor

This PR will:

  • Expose the api request for fetching the users blockchain accounts

@MalcolmnDEV MalcolmnDEV requested a review from cjmconie July 15, 2019 13:49
@MalcolmnDEV MalcolmnDEV changed the base branch from master to dev-open-viewer July 15, 2019 13:50
cjmconie
cjmconie suggested changes Jul 16, 2019
View reviewed changes
Copy link
Contributor

@cjmconie cjmconie left a comment

Choose a reason for hiding this comment

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

The PR looks good, thanks.

Let's add unit test for the model encoding and decoding.
There are some examples for other models. When you include test json, remember to randomise sensitive information, e.g. object ids, wallet addresses, etc.

@MalcolmnDEV MalcolmnDEV requested a review from cjmconie July 19, 2019 07:36
cjmconie
cjmconie reviewed Jul 19, 2019
View reviewed changes
cjmconie
cjmconie suggested changes Jul 19, 2019
View reviewed changes
Copy link
Contributor

@cjmconie cjmconie left a comment

Choose a reason for hiding this comment

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

Please remove the encode and decode functions on the model

@MalcolmnDEV MalcolmnDEV requested a review from cjmconie July 19, 2019 08:59
@MalcolmnDEV MalcolmnDEV merged commit ae47f28 into dev-open-viewer Jul 19, 2019
@cjmconie cjmconie deleted the feature/eth-address branch July 30, 2019 09:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@cjmconie cjmconie cjmconie approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@MalcolmnDEV @cjmconie