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

Adding support for *default fee paying asset* in ApplicationApi #3540

Merged
merged 7 commits into from
Oct 31, 2022

Conversation

ioBanker
Copy link
Member

User had reported that feeAsset is turning into liquidity pool asset while attempting to perform withdraw from a liquidity pool, seems bug is effecting objects.feeAsset.get("id") default object at liquidity pools withdraw modal which should show feeAsset as core asset.

Before fixing the bug:

image_2022-08-29_11-50-43

After fixing the bug:

fix

User had reported that *feeAsset* is turning into *liquidity pool asset* while attempting to perform withdraw from a liquidity pool, seems bug is effecting objects.feeAsset.get("id") default object at liquidity pools withdraw *modal* which should show feeAsset as core asset.
Copy link
Member

@abitmore abitmore left a comment

Choose a reason for hiding this comment

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

This is actually an old issue. I don't think the fix is appropriate though. See #2969

@ioBanker
Copy link
Member Author

ioBanker commented Sep 3, 2022

This is actually an old issue. I don't think the fix is appropriate though. See #2969

I know what you mean hence for now mostly people are paying liquidity pools transaction fees in BTS.

I am considering this: #3001 (comment) I believe its ready but will need to check the bug @sschiessl-bcp is referring to.

@ioBanker
Copy link
Member Author

ioBanker commented Sep 3, 2022

@sschiessl-bcp please pass this for now as some users are not able to pay fees with BTS.

Note: this change is effecting only liquidity removal operation of liquidity pool transaction fees.

@abitmore
Copy link
Member

abitmore commented Sep 3, 2022

Actually I think we should extend the fix in #3492 to all operations: call accountUtils.getFinalFeeAsset() to get an available asset (maybe globally in some way).

@ioBanker
Copy link
Member Author

ioBanker commented Sep 3, 2022

Seems already implemented at loan modal; will check the code there if it can be easily implemented here for all

image

@sschiessl-bcp
Copy link
Contributor

Seems already implemented at loan modal; will check the code there if it can be easily implemented here for all

image

Had time to check?

@ioBanker
Copy link
Member Author

Seems already implemented at loan modal; will check the code there if it can be easily implemented here for all
image

Had time to check?

will be checking this weekend and then update here.

@ioBanker
Copy link
Member Author

Hello @sschiessl-bcp; I'd just checked and such a change would require complex work, for now please merge this fix as it's preventing some liquidity pool users to pay with BTS.

Perhaps another feature implementation like this open case be created later #3088

@abitmore
Copy link
Member

@ioBanker see my comment #3540 (comment)

Actually I think we should extend the fix in #3492 to all operations: call accountUtils.getFinalFeeAsset() to get an available asset (maybe globally in some way).

Don't merge this. Please fix it.

@ioBanker
Copy link
Member Author

@ioBanker see my comment #3540 (comment)

Actually I think we should extend the fix in #3492 to all operations: call accountUtils.getFinalFeeAsset() to get an available asset (maybe globally in some way).

Don't merge this. Please fix it.

Fixed and was tested.

Credits to @thontron for tests

app/api/ApplicationApi.js Outdated Show resolved Hide resolved
@ioBanker
Copy link
Member Author

accountUtils.getFinalFeeAsset(account,"liquidity_pool_withdraw") is successfully working

It can be merged now.

Copy link
Contributor

@sschiessl-bcp sschiessl-bcp left a comment

Choose a reason for hiding this comment

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

Looking at the actual code change, I see the following:

In ApplicationAPI you define

async liquidityPoolWithdraw(
account,
liquidityPoolId,
shareAsset,
shareAmount,
feeAsset = "1.3.0",
broadcast = true
) {

but when calling it you are using

ApplicationApi.liquidityPoolWithdraw(
account,
pool.get("id"),
pool.getIn(['share_asset', "symbol"]),
Math.floor(Number(shareAssetAmount) * Number(sharedAssetPrecision)),
pool.getIn(['asset_a', "symbol"]),
pool.getIn(['asset_b', "symbol"]),
Math.floor(Number(assetAAmount) * Number(assetAPrecision)),
Math.floor(Number(assetBAmount) * Number(assetBPrecision))
)

Please fix the function arguments and implement the fix according to

fee_asset_id = accountUtils.getFinalFeeAsset(
, which has the proper fallback options. Is this maybe the same for deposit?

@ioBanker
Copy link
Member Author

ioBanker commented Oct 3, 2022

Looking at the actual code change, I see the following:

In ApplicationAPI you define

async liquidityPoolWithdraw(
account,
liquidityPoolId,
shareAsset,
shareAmount,
feeAsset = "1.3.0",
broadcast = true
) {

but when calling it you are using

ApplicationApi.liquidityPoolWithdraw(
account,
pool.get("id"),
pool.getIn(['share_asset', "symbol"]),
Math.floor(Number(shareAssetAmount) * Number(sharedAssetPrecision)),
pool.getIn(['asset_a', "symbol"]),
pool.getIn(['asset_b', "symbol"]),
Math.floor(Number(assetAAmount) * Number(assetAPrecision)),
Math.floor(Number(assetBAmount) * Number(assetBPrecision))
)

Please fix the function arguments and implement the fix according to

fee_asset_id = accountUtils.getFinalFeeAsset(

, which has the proper fallback options. Is this maybe the same for deposit?

Good catch; liquidityPoolWithdraw arguments are wrong as assetA and assetB shouldn't be mentioned for this operation unlike deposit, will fix this soon later.

@ioBanker ioBanker closed this Oct 20, 2022
@abitmore abitmore reopened this Oct 20, 2022
@ioBanker ioBanker changed the title Fixing bug in liquidity pools withdraw operation Adding support to *default fee paying asset* in ApplicationApi Oct 24, 2022
@ioBanker ioBanker changed the title Adding support to *default fee paying asset* in ApplicationApi Adding support for *default fee paying asset* in ApplicationApi Oct 24, 2022
@ioBanker
Copy link
Member Author

Below operations execution should be tested while setting a different asset "that has BTS in fee pool" prior merging this:

account_create
transfer
proposal_create
asset_issue
worker_create
withdraw_permission_create
withdraw_permission_update
withdraw_permission_claim
withdraw_permission_delete
vesting_balance_create
ticket_create
liquidity_pool_create
liquidity_pool_delete
liquidity_pool_deposit
liquidity_pool_withdraw
liquidity_pool_exchange

@ioBanker
Copy link
Member Author

ioBanker commented Oct 24, 2022

transfer - Tested
asset_issue - Tested
worker_create - Tested
ticket_create - Tested
liquidity_pool_create - Tested
liquidity_pool_delete - Tested
liquidity_pool_deposit - Tested
liquidity_pool_withdraw - Tested
liquidity_pool_exchange - Tested

I'd merged this change into dex.iobanker.com , please help with testing the other operations.

@ioBanker ioBanker requested review from sschiessl-bcp and abitmore and removed request for sschiessl-bcp October 29, 2022 16:37
Copy link
Member

@abitmore abitmore left a comment

Choose a reason for hiding this comment

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

Thanks.

app/api/ApplicationApi.js Outdated Show resolved Hide resolved
@sschiessl-bcp
Copy link
Contributor

I've changed the behavior to "if fee asset given, then use this, if not, use getFinalFeeAsset"

@sschiessl-bcp sschiessl-bcp merged commit 172f015 into bitshares:develop Oct 31, 2022
@sschiessl-bcp sschiessl-bcp added this to In progress in 5.0.230113 Release via automation Oct 31, 2022
if (!feeAsset) {
// use default fee asset selection if none given
feeAsset = accountUtils.getFinalFeeAsset(
from,
Copy link
Member

Choose a reason for hiding this comment

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

This should be to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants