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

fix: not visible surplus #5968

Merged
merged 16 commits into from Mar 8, 2023
Merged

fix: not visible surplus #5968

merged 16 commits into from Mar 8, 2023

Conversation

jeeanribeiro
Copy link
Contributor

@jeeanribeiro jeeanribeiro commented Feb 24, 2023

Summary

Adds visibility of surplus field when conditions are satisfied, the surplus was not showing before

Changelog

- Adds surplus field in BaseActivity type
- Adds surplus field in GenericActivityInformation
- Adds logic to show visible surplus when necessary

Testing

Platforms

  • Desktop
    • MacOS
    • Linux
    • Windows
  • Mobile
    • iOS
    • Android

Instructions

  • Test each deep link with developer tools

With native token ID

  • should show surplus with 1,000,000 glow & should show storage deposit with 0 glow
    firefly-alpha://wallet/sendConfirmation?address=rms1qpwed3kpaju5rwe4nhplaj3ya6tujzvyuepypya4us95yf2rkp80u5ja230&disableToggleGift=true&disableChangeExpiration=true&amount=1&giftStorageDeposit=true&surplus=1000000&assetId=<NATIVE_TOKEN_ID_THAT_ACCOUNT_IS_HOLDING>

  • should not show surplus & should show storage deposit with 49,600 glow
    firefly-alpha://wallet/sendConfirmation?address=rms1qpwed3kpaju5rwe4nhplaj3ya6tujzvyuepypya4us95yf2rkp80u5ja230&disableToggleGift=true&disableChangeExpiration=true&amount=1&giftStorageDeposit=true&assetId=<NATIVE_TOKEN_ID_THAT_ACCOUNT_IS_HOLDING>

  • should show surplus with 49,000 glow & should show storage deposit with 49,600 glow
    firefly-alpha://wallet/sendConfirmation?address=rms1qpwed3kpaju5rwe4nhplaj3ya6tujzvyuepypya4us95yf2rkp80u5ja230&disableToggleGift=true&disableChangeExpiration=true&amount=1&giftStorageDeposit=true&surplus=49000&assetId=<NATIVE_TOKEN_ID_THAT_ACCOUNT_IS_HOLDING>

  • should show surplus with 60,000 glow & no storage deposit
    firefly-alpha://wallet/sendConfirmation?address=rms1qpwed3kpaju5rwe4nhplaj3ya6tujzvyuepypya4us95yf2rkp80u5ja230&disableToggleGift=true&disableChangeExpiration=true&amount=1&surplus=60000&assetId=<NATIVE_TOKEN_ID_THAT_ACCOUNT_IS_HOLDING>

Without native token ID

  • should show surplus not supported error
    firefly-alpha://wallet/sendConfirmation?address=rms1qpwed3kpaju5rwe4nhplaj3ya6tujzvyuepypya4us95yf2rkp80u5ja230&disableToggleGift=true&disableChangeExpiration=true&amount=1&giftStorageDeposit=true&surplus=1000000

  • should not show surplus and should show 42,599 in storage deposit
    firefly-alpha://wallet/sendConfirmation?address=rms1qpwed3kpaju5rwe4nhplaj3ya6tujzvyuepypya4us95yf2rkp80u5ja230&disableToggleGift=true&disableChangeExpiration=true&amount=1&giftStorageDeposit=true

  • should show surplus not supported error
    firefly-alpha://wallet/sendConfirmation?address=rms1qpwed3kpaju5rwe4nhplaj3ya6tujzvyuepypya4us95yf2rkp80u5ja230&disableToggleGift=true&disableChangeExpiration=true&amount=1&giftStorageDeposit=true&surplus=49000

  • should show surplus not supported error
    firefly-alpha://wallet/sendConfirmation?address=rms1qpwed3kpaju5rwe4nhplaj3ya6tujzvyuepypya4us95yf2rkp80u5ja230&disableChangeExpiration=true&amount=1&surplus=50000

Checklist

  • I have followed the contribution guidelines for this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or modified tests that prove my changes work as intended
  • I have verified that new and existing unit tests pass locally with my changes
  • I have verified that my latest changes pass CI workflows for testing and linting
  • I have made corresponding changes to the documentation

@jeeanribeiro jeeanribeiro added type:fix Fix for bug package:desktop stardust Related to the Stardust Protocol labels Feb 24, 2023
@jeeanribeiro jeeanribeiro self-assigned this Feb 24, 2023
@jeeanribeiro jeeanribeiro linked an issue Feb 24, 2023 that may be closed by this pull request
3 tasks
@nicole-obrien
Copy link
Contributor

The second test case is not showing storage deposit, nor is it showing an asset. This leads to the following transaction: https://explorer.shimmer.network/testnet/transaction/0xed58b2af79a2ef81bed56dc41d398e00c4a338497e05003b3a7d7a7eb37a030c
Screenshot 2023-02-27 at 11 34 33

Perhaps the other prs need to be merged first??

@nicole-obrien
Copy link
Contributor

nicole-obrien commented Feb 27, 2023

2,3,4 test cases are not working

@maxwellmattryan
Copy link
Contributor

Only test case 2 is not working now

@jeeanribeiro
Copy link
Contributor Author

jeeanribeiro commented Feb 28, 2023

@maxwellmattryan @nicole-obrien I'm sorry, I've removed the assetId param from the deep links by mistake, it should test the native tokens, now it should work, you just need to put an assetId that the account is holding

@nicole-obrien
Copy link
Contributor

Im not sure the expected results of the last test case are actually correct. Lets look at the test cases tomorrow.

Copy link
Contributor

@maxwellmattryan maxwellmattryan left a comment

Choose a reason for hiding this comment

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

Code looks good. Some feedback:

  • On the third test case, it only showed 600 glow rather than 49,600.
  • If the storage deposit is zero, then imo we shouldn't show the "Gift storage deposit" key value box in the send confirmation popup. It's interesting because this is happening in the 6th case but in the 4th case it's the opposite.
  • Cases 5 and 7 are showing surpluses and 0 for the storage deposits. Case 8 shows the correct storage deposit but it also shows the surplus.

Copy link
Contributor

@maxwellmattryan maxwellmattryan left a comment

Choose a reason for hiding this comment

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

LGTM 🔥

@maxwellmattryan maxwellmattryan merged commit 3eaecdb into develop Mar 8, 2023
4 checks passed
@maxwellmattryan maxwellmattryan deleted the fix/not-visible-surplus branch March 8, 2023 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stardust Related to the Stardust Protocol type:fix Fix for bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Task]: Fix surplus amount not showing when following a deep link
3 participants