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.

Sign up for GitHub

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

Jump to bottom

AND-9157 Fact0rn support #844

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

AND-9157 Fact0rn support #844

wants to merge 2 commits into from
+523 −32

Conversation

nemelianov-tangem
Copy link
Contributor

@nemelianov-tangem nemelianov-tangem commented Nov 25, 2024 •
edited
Loading

@nemelianov-tangem nemelianov-tangem requested review from dbaturin and a team as code owners November 25, 2024 06:48
@nemelianov-tangem nemelianov-tangem mentioned this pull request Nov 25, 2024
Open
3 tasks
MasterBin
MasterBin previously approved these changes Nov 25, 2024
View reviewed changes
kozarezvlad
kozarezvlad previously approved these changes Nov 25, 2024
View reviewed changes
Mama1emon
Mama1emon previously approved these changes Nov 28, 2024
View reviewed changes
@nemelianov-tangem nemelianov-tangem marked this pull request as draft November 28, 2024 14:56
@nemelianov-tangem nemelianov-tangem changed the title AND-9157 Fact0rn support empty classes AND-9157 Fact0rn support Dec 2, 2024
@nemelianov-tangem nemelianov-tangem marked this pull request as ready for review December 2, 2024 08:13
@nemelianov-tangem nemelianov-tangem force-pushed the feature/AND-9157_fact0rn_support branch 4 times, most recently from 1c1242a to d35a418 Compare December 5, 2024 09:42
@nemelianov-tangem nemelianov-tangem marked this pull request as draft December 5, 2024 10:08
@nemelianov-tangem nemelianov-tangem force-pushed the feature/AND-9157_fact0rn_support branch 2 times, most recently from 465dc7d to 1523ef0 Compare December 7, 2024 04:45
@nemelianov-tangem nemelianov-tangem changed the base branch from develop to release-app_5.19 December 7, 2024 04:51
@nemelianov-tangem nemelianov-tangem marked this pull request as ready for review December 7, 2024 04:52
@nemelianov-tangem nemelianov-tangem changed the base branch from release-app_5.19 to develop December 9, 2024 09:29
kozarezvlad
kozarezvlad previously approved these changes Dec 10, 2024
View reviewed changes
@nemelianov-tangem nemelianov-tangem force-pushed the feature/AND-9157_fact0rn_support branch from 1523ef0 to 699e152 Compare December 11, 2024 08:49
nzeeei
nzeeei previously approved these changes Dec 11, 2024
View reviewed changes
dbaturin
dbaturin reviewed Dec 11, 2024
View reviewed changes
...ain/src/main/java/com/tangem/blockchain/blockchains/factorn/network/Fact0rnNetworkService.kt Outdated
Comment on lines 165 to 167
private const val MINIMAL_NUMBER_OF_CONFIRMATION_BLOCKS = 25
private val NORMAL_FEE_MULTIPLIER = 2.5.toBigDecimal()
private val PRIORITY_FEE_MULTIPLIER = 5.toBigDecimal()
Copy link
Contributor

Choose a reason for hiding this comment

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

Откуда эти значения взялись? В Radiant используется Electrum, блоки так же часто, так почему здесь другие? Особенно основное - MINIMAL_NUMBER_OF_CONFIRMATION_BLOCKS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

в исходниках константы кол-ва блоков И как используют раз и два Я взял 25 10 5 для minimal normal priority соотвественно

Copy link
Contributor

Choose a reason for hiding this comment

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

Не, это плохие цели по блокам. 25 блоков - это 4.5 часа. По биткоину (а время блока то же там) в провайдере, который использует количество блоков, мы запрашиваем с таким целевым количеством:

const val MINIMAL_FEE_BLOCK_AMOUNT = 8
const val NORMAL_FEE_BLOCK_AMOUNT = 4
const val PRIORITY_FEE_BLOCK_AMOUNT = 1

Умножение при этом не даёт предсказуемого эффекта. По хорошему, надо делать 3 запроса с разным целевым параметром этим. Если с этим какие-то проблемы (апишка тупит мб), то можно попробовать с параметрами как в Radiant домножать. Но лучше всё же 3 запроса

Copy link
Contributor Author

Choose a reason for hiding this comment

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

поправил кол-во блоков и добавил три запроса, при ошибке сделал через умножение

dbaturin
dbaturin requested changes Dec 11, 2024
View reviewed changes
Copy link
Contributor

@dbaturin dbaturin left a comment

Choose a reason for hiding this comment

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

Проблема для исправления в отдельном комменте выше

@nemelianov-tangem nemelianov-tangem force-pushed the feature/AND-9157_fact0rn_support branch 2 times, most recently from 303a3c9 to bb85717 Compare December 12, 2024 06:14
@nemelianov-tangem nemelianov-tangem force-pushed the feature/AND-9157_fact0rn_support branch from bb85717 to 84ccf34 Compare December 12, 2024 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kozarezvlad kozarezvlad kozarezvlad approved these changes

@MasterBin MasterBin MasterBin left review comments

@Mama1emon Mama1emon Mama1emon left review comments

@nzeeei nzeeei nzeeei left review comments

@Sateetas Sateetas Awaiting requested review from Sateetas Sateetas is a code owner automatically assigned from tangem/android-reviewers-test

@dbaturin dbaturin Awaiting requested review from dbaturin dbaturin is a code owner

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nemelianov-tangem @kozarezvlad @nzeeei @Mama1emon @MasterBin @dbaturin