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

Subtract pending outbound tx amounts from available balance #1701

Closed
kyranjamie opened this issue Sep 10, 2021 · 22 comments · Fixed by #2413
Closed

Subtract pending outbound tx amounts from available balance #1701

kyranjamie opened this issue Sep 10, 2021 · 22 comments · Fixed by #2413

Comments

@kyranjamie
Copy link
Collaborator

For example, when sending a tx and then using the Send max functionality, the outbound pending tx isn't considered.

@fbwoolf
Copy link
Contributor

fbwoolf commented Sep 23, 2021

There has been conversation around not doing this, but it is still in the backlog? Should this be started? @aulneau I think you challenged not doing it?

@markmhendrickson
Copy link
Collaborator

I presume we still want this prioritized for the backlog, since we do this in the desktop wallet already and it seems to work well (and I don't recall the recent convo around not doing it). But curious to hear what @aulneau thinks if he has concerns.

@markmhendrickson
Copy link
Collaborator

@timstackblock to retest

@markmhendrickson markmhendrickson moved this from Backlog to QA in Hiro Wallet (DEPRECATED) May 2, 2022
@Eshwari007
Copy link

Findings:
For example, when sending a tx and then using the Send max functionality, the outbound pending tx isn't considered.

The use case mentioned above by @kyranjamie hasn't been fixed.

Testing scenario:

  • I transferred STX from account 1 to account 2
  • Tx is pending in account 1.

Results:

  • Before the tx, I tested the account send max functionality with Low fee rate
  • After the tx, I retested the same account send max functionality with Low fee rate

They were the same amount which means that the Send Max didn't take into the account the STX tx which is yet pending

Screen Shot 2022-05-03 at 4 00 21 PM

Screen Shot 2022-05-03 at 4 00 28 PM

Screen Shot 2022-05-03 at 4 04 17 PM

cc @markmhx
Moving it back to the backlog.

@Eshwari007 Eshwari007 moved this from QA to Backlog in Hiro Wallet (DEPRECATED) May 3, 2022
@markmhendrickson
Copy link
Collaborator

@fbwoolf want to take this one?

@fbwoolf
Copy link
Contributor

fbwoolf commented May 4, 2022

@fbwoolf want to take this one?

Yep, for sure. 👍

@fbwoolf fbwoolf self-assigned this May 4, 2022
@fbwoolf fbwoolf moved this from Backlog to Development in Hiro Wallet (DEPRECATED) May 10, 2022
@fbwoolf
Copy link
Contributor

fbwoolf commented May 11, 2022

@kyranjamie I'm working on this, but do you think we should ask the api team to add a pending_balance to /extended/v1/address/{principal}/stx?

@fbwoolf
Copy link
Contributor

fbwoolf commented May 11, 2022

Also, are we wanting to subtract out just pending txs or locally 'submitted' tx balances too?

EDIT: I put up a PR, so just did pending txs unless CR says otherwise.

@fbwoolf fbwoolf linked a pull request May 11, 2022 that will close this issue
@fbwoolf fbwoolf moved this from Development to Review in Hiro Wallet (DEPRECATED) May 11, 2022
@markmhendrickson
Copy link
Collaborator

If we expect submitted balances to eventually go through as pending, then I presume it'd be best to subtract them as well?

@kyranjamie
Copy link
Collaborator Author

I'd just use the mempool amounts to subtract "pending balances". We could also subtract submitted amounts, but this should only ever be a short-lived state, and sometimes it goes wrong, so I would avoid this to begin with. I don't think we need a new API prop.

This is how it's done in desktop wallet https://github.com/hirosystems/stacks-wallet/blob/d35bffa0a072a7d03212004f260bc59ff1259ea5/app/utils/tx-utils.ts#L67-L69

@fbwoolf fbwoolf moved this from Review to QA in Hiro Wallet (DEPRECATED) May 12, 2022
@Eshwari007 Eshwari007 self-assigned this May 13, 2022
@Eshwari007
Copy link

Quick questions:

@fbwoolf

  • Is the use case above applicable when the user selects Maxamount only?
  • Can the user input a specific amount and test the available balance with the pending outbound as well?

@Eshwari007
Copy link

As I was testing Max amount ,noticed the following:

  • User send from account A to B with max amount of STX
  • User notices that the tx is Pending
  • User then selects account A-STX and when try to selects MAX,there were no action( no values were presented in the stx amount box)
  • Though I understand the amount has been subtracted but felt it can be a little confusing when a user sees the balance value of STX yet visible(though the max tx is pending), no amount is presented when the user selects MAX.

The screen recording below can explain the scenario better.

Max.amount.tx.pending.mov

cc @markmhx @fbwoolf

@fbwoolf
Copy link
Contributor

fbwoolf commented May 13, 2022

The screen recording below can explain the scenario better.

Yep, that is expected but not sure if there is more info we want to provide if the pending tx is using all available STX? We could def provide an error message below the input field in this situation?

@Eshwari007
Copy link

Eshwari007 commented May 13, 2022

The screen recording below can explain the scenario better.

Yep, that is expected but not sure if there is more info we want to provide if the pending tx is using all available STX? We could def provide an error message below the input field in this situation?

Yes, highly agree with you in providing an error message below the input field in this situation, esp for new users/beginners -the info will be helpful

@markmhx any inputs on the error message or warnings for the scenario mentioned above.

@Eshwari007
Copy link

Also is this applicable only for STX?
I did not see the max amount change for non STX token such as STSW and Wrapped nothing(these are tokens on the test account)

@fbwoolf
Copy link
Contributor

fbwoolf commented May 13, 2022

Also is this applicable only for STX? I did not see the max amount change for non STX token such as STSW and Wrapped nothing(these are tokens on the test account)

Yep, this is only for STX.

@Eshwari007
Copy link

Also is this applicable only for STX? I did not see the max amount change for non STX token such as STSW and Wrapped nothing(these are tokens on the test account)

Yep, this is only for STX.

Is there any future enhancement plan for the non-Stx tokens?
@markmhx

@Eshwari007
Copy link

Test passed except for the error message/warning which will be ideal for the use case above.
Pinged Mark on it along with a potential enhancement ticket for the non STX ticket

@markmhendrickson
Copy link
Collaborator

Ideally we should have this for STX and non-STX assets, even if we release both as separate enhancements. @Eshwari007 please do create a separate ticket for non-STX assets 🙏

@Eshwari007
Copy link

Eshwari007 commented May 16, 2022

Ideally we should have this for STX and non-STX assets, even if we release both as separate enhancements. @Eshwari007 please do create a separate ticket for non-STX assets 🙏

How abt the error message or warning when user selects Max while a pending tx with full Max tx is in the process as well thus has no inputs when the user selects the Max button.( the scenario on the video above)
cc @markmhx

@markmhendrickson
Copy link
Collaborator

Let's create a separate issue for this message as an enhancement 🙏

@Eshwari007
Copy link

#2418
#2417

The above tickets were created as Enhancements to address QA's findings.

@Eshwari007 Eshwari007 moved this from QA to Ready to merge in Hiro Wallet (DEPRECATED) May 16, 2022
@fbwoolf fbwoolf moved this from Ready to merge to Closed in Hiro Wallet (DEPRECATED) May 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants