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

Live balances, transactions and prices #9

Merged
merged 73 commits into from
Jan 27, 2022
Merged

Conversation

chescalante
Copy link
Contributor

@chescalante chescalante commented Dec 14, 2021

  • connect by address
  • handle disconnect
  • polling strategy for tokens
  • push transactions
  • (push tokens)
  • polling strategy for prices
  • cache for prices

@chescalante chescalante marked this pull request as ready for review December 17, 2021 12:19
src/index.ts Outdated Show resolved Hide resolved
@@ -3,6 +3,9 @@ import express, { Request, Response } from 'express'
import { Api } from './api'
import registeredDapps from './registered_dapps'
import { isValidAddress } from './utils'
import { Server } from 'socket.io'
import http from 'http'

Choose a reason for hiding this comment

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

Suggested change
import http from 'http'
import https from 'https'

Lets make this safe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll need a ssl certificate for that. (https://nodejs.org/en/knowledge/HTTP/servers/how-to-create-a-HTTPS-server/)
Let's do it in another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

issue: #12

Choose a reason for hiding this comment

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

yes, definitely.
For dev you can use a self signed (self generated) for prod, you should cordinate that with devops

Copy link
Collaborator

@ilanolkies ilanolkies left a comment

Choose a reason for hiding this comment

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

Great job! Most of the corrections are about modeling. Should we prepare a better definition before addressing the changes?

src/api/index.ts Outdated Show resolved Hide resolved
src/api/index.ts Outdated Show resolved Hide resolved
src/coinmatketcap/cache.ts Outdated Show resolved Hide resolved
src/coinmatketcap/cache.ts Outdated Show resolved Hide resolved
id: params.addresses.map(address => addressToCoinmarketcapId[address]).join(','),
const fromQueryParamsToRequestParams = (params: PricesQueryParams, chaindId: number): ICoinMarketCapQuoteParams => ({
id: params.addresses
.filter((address) => isTokenSupported(address, chaindId))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this again? We should do decide where to do it and do it only once

src/coinmatketcap/support.ts Outdated Show resolved Hide resolved
}
}

const executeFactory = (
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess we wan to declare the functions before we use them. Should we reorder this code?

src/subscriptions/pushNewPrices.ts Outdated Show resolved Hide resolved
src/subscriptions/pushNewTransactions.ts Outdated Show resolved Hide resolved
src/subscriptions/pushNewTransactions.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@ilanolkies ilanolkies left a comment

Choose a reason for hiding this comment

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

Good job team! Code looks good. We can take this as a starting point for the refactor. There are some things to fix, I am not receiving transactions and prices events.

sleyter93 and others added 4 commits January 26, 2022 15:39
* Custom error class and errorhandler functio

* Removing errorhandler from here and using it in setupAPI

* Uncommenting the asserts that checked the text of the error message response

* Implementing next function in the error handlers of the promises

* Update src/middleware/index.ts

Co-authored-by: Ilan <36084092+ilanolkies@users.noreply.github.com>

* Update src/middleware/index.ts

Co-authored-by: Ilan <36084092+ilanolkies@users.noreply.github.com>

* Update src/api/index.ts

Co-authored-by: Ilan <36084092+ilanolkies@users.noreply.github.com>

* applying suggestions

Co-authored-by: Agustin Villalobos <agustin.villalobos@iovlabs.org>
Co-authored-by: Ilan <36084092+ilanolkies@users.noreply.github.com>
Fix how transactions are selected to be pushed
Copy link
Collaborator

@ilanolkies ilanolkies left a comment

Choose a reason for hiding this comment

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

Great job team!!!

@ilanolkies ilanolkies merged commit c8f69e1 into develop Jan 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants