-
Notifications
You must be signed in to change notification settings - Fork 289
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
[Fix/stacks] Optimistic operation disappears for long running transactions #6793
base: develop
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git βοΈ
3 Ignored Deployments
|
@lawRathod is attempting to deploy a commit to the LedgerHQ Team on Vercel. A member of the Team first needs to authorize it. |
There as been no activity on this PR for the last 14 days. Please consider closing this PR. |
c2c41cc
to
9e9caf7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see my comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks good to me.
I've some questions,
I don't know whether the balance return from /extended/v1/address/${addr}/stx includes the pending transactions(mempool transactions)
I see this piece of code
for (const tx of mempoolTxs) {
spendableBalance = spendableBalance
.minus(new BigNumber(tx.fee_rate))
.minus(new BigNumber(tx.token_transfer.amount));
}
Does the mempoolTxs includes both incoming and outgoing (send and receive) transactions?
because the "fee_rate" is not paid by the current account for receive transactions.
should we give the "tx.token_transfer.amount" a positive or negative value according to send/receive transaction?
@hzheng-ledger Thanks for the review.
Only sent transactions are part of the mempool txns object https://github.com/Zondax/ledger-live/blob/develop/libs/ledger-live-common/src/families/stacks/bridge/utils/api.ts#L142-L147 so we substract the fee and amount from the balance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks good to me. thanks
@@ -206,6 +263,7 @@ export const getAccountShape: GetAccountShape = async info => { | |||
balance, | |||
spendableBalance, | |||
operations: flatMap(rawTxs, mapTxToOps(accountId)), | |||
pendingOperations: flatMap(mempoolTxs, mapPendingTxToOps(accountId, address)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we shouldn't add pendingOperations here.
This pendingOperations is used for new broadcasted transaction from our UI. (not even necessarily in mempool, but we want to display them to users in our Ledger live) It is for different usage.
what we should do here is to merge flatMap(rawTxs, mapTxToOps(accountId))
and flatMap(mempoolTxs, mapPendingTxToOps(accountId, address))
in the above "operations" field. for confirmed transaction, we set "blockHeight" and "blockHash" field correctly and for pending transaction in mempool, we set "blockHeight" and "blockHash" field as null. So that we can see these pending transactions in our Ledger live
the "pendingOperations" is normally not used in "getAccountShape" method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the explanation, there was definitely some confusion around this but everything is much clear now. I have merged both as you suggest, please have another look ππ»
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see my another comment. Thanks a lot
9e9caf7
to
8b6d0af
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me
Thanks a lot. I've approved the PR |
8b6d0af
to
315c757
Compare
β Checklist
npx changeset
was attached.π Description
The optimistics transaction disappears when transactions on stacks network takes long time. The pending operations are now added to accounts object during sync to fix the problem.
β Context
π§ Checklist for the PR Reviewers