Skip to content
This repository has been archived by the owner on Dec 26, 2022. It is now read-only.

Implement api_generate_address #19

Merged
merged 4 commits into from
Dec 21, 2018

Conversation

jkrvivian
Copy link
Member

@jkrvivian jkrvivian commented Dec 21, 2018

Close #6

@jserv
Copy link
Member

jserv commented Dec 21, 2018

Use Close #6 instead of Fix #6 since you are implementing something.

}

ta_generate_address_res_serialize(&json_result, res);
if (ret) {
Copy link
Member

Choose a reason for hiding this comment

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

Will ret be changed in previous line?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes! Fixed.

Copy link
Member

Choose a reason for hiding this comment

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

If goto is used right after if (ret), this function would look lighter.

Copy link

Choose a reason for hiding this comment

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

Also the last check is not needed I think

wusyong
wusyong previously approved these changes Dec 21, 2018
@wusyong wusyong dismissed their stale review December 21, 2018 09:27

Should review updated commit

@wusyong wusyong self-requested a review December 21, 2018 09:31

ret = ta_generate_address_res_serialize(&json_result, res);
if (ret) {
goto done;
Copy link
Member

Choose a reason for hiding this comment

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

This goto is not necessary here.

@jserv
Copy link
Member

jserv commented Dec 21, 2018

Rebasing is required.

@jserv jserv merged commit d87ad6b into DLTcollab:master Dec 21, 2018
@jkrvivian jkrvivian deleted the api_generate_address branch December 22, 2018 05:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants