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(minifront): #1598: display non-native-fee-warning per account #1614

Merged
merged 10 commits into from
Aug 2, 2024

Conversation

VanishMax
Copy link
Contributor

Closes #1598

Checks if user has UM token in the same account as the selected source token

Kapture.2024-08-01.at.14.24.00.mp4

@VanishMax VanishMax requested a review from a team August 1, 2024 12:29
@VanishMax VanishMax self-assigned this Aug 1, 2024
Copy link

changeset-bot bot commented Aug 1, 2024

⚠️ No Changeset found

Latest commit: f3c73d2

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@Valentine1898
Copy link
Contributor

That can be confusing for users because not every asset can be an alt fee asset
In your demo TestUSD is not an alt fee asset, and it is not the case when a non-native fee asset is used

@VanishMax
Copy link
Contributor Author

@Valentine1898 do I get it right that, if user has TestUSD (non-alt token) and, let's say, AltUSD (some alt token), and the source token is TestUSD, the fees will still be paid in AltUSD, if the amount is enough to cover the fees?

@Valentine1898
Copy link
Contributor

@Valentine1898 do I get it right that, if user has TestUSD (non-alt token) and, let's say, AltUSD (some alt token), and the source token is TestUSD, the fees will still be paid in AltUSD, if the amount is enough to cover the fees?

Yes, that's right

@VanishMax
Copy link
Contributor Author

@Valentine1898 @TalDerei I implemented the checks for alt token balances before showing the NonNativeFeeWarning, mainly copied the logic from getAssetFromGasPriceTable service function. Ready for review.

Right now, viewClient.gasPrices({}) returns empty array of altGasPrices for both testnet and penumbra-1. Any idea how to test it locally?

@TalDerei
Copy link
Contributor

TalDerei commented Aug 2, 2024

Right now, viewClient.gasPrices({}) returns empty array of altGasPrices for both testnet and penumbra-1. Any idea how to test it locally?

to simulate gas prices, add this code block inside the block processor and resync, then check updated GasPrices table.

const assetId1 = new AssetId({
      inner: base64ToUint8Array("drPksQaBNYwSOzgfkGOEdrd4kEDkeALeh58Ps+7cjQs=")
    });

    const assetId2 = new AssetId({
      inner: base64ToUint8Array("B+9mATKkwyNfqyctQ9m5dSqDN7LRCFl6v/r/XyRtDw8=")
    });

    const assetId3 = new AssetId({
      inner: base64ToUint8Array("KSOgqHs6JCHxZcyFPb9zqb2vtdoNlIVktgWcsCF8RAc=")
    });

    await this.indexedDb.saveGasPrices(
      new GasPrices({
        assetId: this.stakingAssetId,
        blockSpacePrice: 60n,
        compactBlockSpacePrice: 1556n,
        verificationPrice: 1n,
        executionPrice: 1n
      }),
    );

    await this.indexedDb.saveGasPrices(
      new GasPrices({
        assetId: assetId1,
        blockSpacePrice: 600n,
        compactBlockSpacePrice: 15560n,
        verificationPrice: 10n,
        executionPrice: 10n
      }),
    );

    await this.indexedDb.saveGasPrices(
      new GasPrices({
        assetId: assetId2,
        blockSpacePrice: 120n,
        compactBlockSpacePrice: 3112n,
        verificationPrice: 2n,
        executionPrice: 2n
      }),
    );

    await this.indexedDb.saveGasPrices(
      new GasPrices({
        assetId: assetId3,
        blockSpacePrice: 1200n,
        compactBlockSpacePrice: 31120n,
        verificationPrice: 20n,
        executionPrice: 20n
      }),
    );

    await this.indexedDb.saveGasPrices(
      new GasPrices({
        assetId: this.stakingAssetId,
        blockSpacePrice: 60n,
        compactBlockSpacePrice: 1556n,
        verificationPrice: 1n,
        executionPrice: 1n
      }),
    );

@Valentine1898
Copy link
Contributor

@VanishMax
It's an RPC bug, here's the fix
#1626

@Valentine1898 Valentine1898 mentioned this pull request Aug 2, 2024
@Valentine1898
Copy link
Contributor

Please rebase on main, I'll review later today

@VanishMax
Copy link
Contributor Author

@Valentine1898 Updated demo where I added TestUSD to the alt tokens in IndexedDB and showing 3 cases:

  1. Account 10 with TestUSD and no UM tokens – warning shows correctly
  2. Account 12 with Osmo (not an alt token) – no warning as user can't pay for alt fees this way (we might consider adding validation for this case later)
  3. Account 0 (with UM tokens) with GN – no warning as UM will be used to pay for the fees
Kapture.2024-08-02.at.15.24.54.mp4

@TalDerei
Copy link
Contributor

TalDerei commented Aug 2, 2024

@Valentine1898 Updated demo where I added TestUSD to the alt tokens in IndexedDB and showing 3 cases:

these seem like the proper expected cases to me 👍

Copy link
Contributor

@Valentine1898 Valentine1898 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!

One suggestion, let's not show a warning for Send if the user has not entered a recipient address, so as not to confuse the user

image

);
};

export const useShouldRender = (balancesResponses: BalancesResponse[] = [], amount: number) => {
// Finds the alt tokens in the user's account balances that can be used for fees
const hasAltToken = (
Copy link
Contributor

Choose a reason for hiding this comment

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

can we combine hasStakingToken and hasAltToken into hasTokenBalance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

refactored the code now + removed useShouldRender since there's no need in it after creating hasTokenBalance

return false;
}

const sourceAddressIndex = getAddressIndex.optional()(account)?.account ?? 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we have a default value for source here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no need, just removed it

* required.
* Renders a non-native fee warning if
* 1. the user does not have any balance (in the selected account) of the staking token to use for fees
* 2. user has alt token balances to pay for the fees
Copy link
Contributor

Choose a reason for hiding this comment

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

"The user does not have sufficient balances in alternative tokens to cover the fees."

@TalDerei TalDerei self-requested a review August 2, 2024 14:38
@VanishMax VanishMax merged commit f983923 into main Aug 2, 2024
6 checks passed
@VanishMax VanishMax deleted the fix/#1598-non-native-fees-per-account branch August 2, 2024 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The NonNativeFeeWarning is never displayed
3 participants