-
Notifications
You must be signed in to change notification settings - Fork 51
(cosmos) Fix max spend #988
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
base: master
Are you sure you want to change the base?
Conversation
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const totalCost = add(mid, networkFee) | ||
| if (gt(totalCost, balance)) { | ||
| return await getMax(min, mid) |
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.
Handle gas balance separately for token max spend
When computing the maximum spendable amount for Cosmos tokens, the code adds the fee (denominated in the native gas token) to the token amount and compares that sum with the token balance. This has two consequences: the returned token amount is reduced by the fee even though the fee is paid in the native currency, and worse, if the wallet lacks native coins to cover the fee the method will still return the full token balance as spendable. Users attempting to send a token with zero native balance will therefore see a non‑zero max amount and then hit an “insufficient fee” failure during makeSpend. The fee needs to be checked against the native balance instead of being combined with the token amount.
Useful? React with 👍 / 👎.
swansontec
left a comment
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.
The core's default implementation calls makeSpend in a binary search. How does this really differ? Also, there is the issue AI found where the fee gets added to the token value - seems like a units mix-up?
|
|
||
| async getMaxSpendable(spendInfo: EdgeSpendInfo): Promise<string> { | ||
| const { memos = [], tokenId } = spendInfo | ||
| const { currencyCode } = |
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.
Just delete the currencyCode. It's not really used.
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
noneDescription
noneNote
Adds an iterative, fee-estimating
getMaxSpendableto Cosmos engine (incl. IBC cases) and updates CHANGELOG.src/cosmos/engine/CosmosEngine.ts):getMaxSpendable(spendInfo)using iterative (binary search) fee estimation viacalculateFee, supporting memos, tokens, and IBC transfers withvalidateTransfer.divandlte.Written by Cursor Bugbot for commit b289105. This will update automatically on new commits. Configure here.