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

Create utilities to connect to btc - Closes #1895 #1920

Merged
merged 9 commits into from Apr 15, 2019

Conversation

Projects
None yet
4 participants
@massao
Copy link
Contributor

commented Apr 11, 2019

What issue have I solved?

#1895

How have I implemented/fixed it?

Copied the utilities from the lisk-mobile repository to be able to connect to the BTC node.

  • Not saving nothing in the store right now, also not calling the utilities from any file.
  • Had to change the code a little, due to we having the network switcher on Hub.
    • It will connect to the based on the netCode passed to the functions, by default it's 1 (testnet).
    • If connected to a custom lisk node it connect to the btc testnet.
  • I was able to test the functions for fetching some account information, with the extractAddress and getAccount functions that are being exported temporarily from the api/accounts.js file.
  • Also changed from plain fetch to the popsicle lib similar to we do with the lskService.

If you want to try the utilities, you can use some existing action and import the functions and call from there.

Refactors needed

  • See which api utilities are "shared" between multiple currencies and use the function mapper to map to the correct one, and have only one export point for the mapped function.
  • Update to final Services API once it's done by mobile team.

Known issues

  • The btc functions right now are all skipped on unit tests due to it using other APIs that we would need to mock, and since we also skipped the services API we don't have a example to use, maybe a point to be discussed on next technical meeting.

How has this been tested?

Created unit tests to helper functions, and tested that we are able to connect and fetch some data from the btc testnet.
you can update the accountDataUpdated action to use the btc.extractAddress and btc.getAccount exported by api/account, to get the account summary from testnet.

Review checklist

massao added some commits Apr 9, 2019

@massao massao self-assigned this Apr 11, 2019

massao added some commits Apr 11, 2019

@massao massao requested a review from altayaydemir Apr 11, 2019

@massao massao requested review from slaweet and osvaldovega Apr 12, 2019

Update src/utils/api/btc/config.js to use service bridge API
Co-Authored-By: massao <rmy.yonamine@gmail.com>

massao added some commits Apr 12, 2019

@altayaydemir
Copy link
Contributor

left a comment

👍 looks good

@osvaldovega
Copy link
Contributor

left a comment

man looks good, I think after Altay checked there are not much to say.
The only thing that I really don't know is that un the file src/utils/api/btc/transactions.js there are some names that not clear, probably when you read it or wrote it had sense but after check some names I really dont get what are those variables/functions... I dont know what do you think?

}
});

export const create = ({

This comment has been minimized.

Copy link
@osvaldovega

osvaldovega Apr 15, 2019

Contributor

I think this function could be split in more functions, there are a lot of code inside this function that do different things, so maybe those could be in a different place

This comment has been minimized.

Copy link
@massao

massao Apr 15, 2019

Author Contributor

Yeah, I was discussing this with @aydieneue on Friday, but we are not really sure on how to split this function without turning this more complex or what kind of logic would make sense to not keep on this function and move to a separate function, since in the end all the logic are only used to create the transaction.

@osvaldovega
Copy link
Contributor

left a comment

🎼nice work

@osvaldovega osvaldovega added the ready label Apr 15, 2019

@slaweet slaweet merged commit aaec25c into 1.16.0 Apr 15, 2019

4 checks passed

Jenkins e2e tests e2e tests passed
Details
Jenkins test deployment Commit was deployed to test
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
security/snyk - package.json (LiskHQ) No new issues
Details

@slaweet slaweet deleted the 1895-create-utilities-to-connect-to-BTC branch Apr 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.