feat: make a support of Qt app to show Platform transfer Tx

knst commented 1 month ago

Issue being fixed or feature implemented

Transfers from platform have incorrectly shown amount in Dash Core wallet app. They also shown in Qt app as self-send that is not completely true.

What was done?

Added new type of transaction to Qt App, added a filter for its type, fixed calculation of output for tx records. As well added a new type of transaction platform-transfer in rpc output of gettransaction RPC

How Has This Been Tested?

Make a Platform Transfer transaction on RegTest and check it in Dash Core

Helper to see it: export dpath=/tmp/dash_func_test_PATHPATH/ ; src/qt/dash-qt -regtest -conf=$dpath/node0/dash.conf -datadir=$dpath/node0/ -debug=0 -debuglogfile=/dev/stdout

Breaking Changes

There's new type of transaction "platform-transfer" in rpc output of gettransaction.

This PR DOES NOT change any consensus rules. Breaking changes that makes withdrawal transaction immature is moved to https://github.com/dashpay/dash/pull/6128

Checklist:

[x] 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 updated relevant unit/integration/functional/e2e tests
[ ] I have made corresponding changes to the documentation
[x] I have assigned this pull request to a milestone

kxcd commented 1 month ago

The term withdrawal will be confusing. Suggest different name eg Asset pool to denote it comes from an asset pool, or more specifically Platform to indicate this is coming from Platform. It will be clear this a credit to the wallet, so no need to muddy the waters with terms like withdrawal.

knst commented 1 month ago

@thephez

The term withdrawal will be confusing.

Could you suggest? just "Asset unlock" is not good too. Internally we already call it asset unlock or withdrawal.

thephez commented 1 month ago

@thephez

The term withdrawal will be confusing.

Could you suggest? just "Asset unlock" is not good too. Internally we already call it asset unlock or withdrawal.

Yes, we should think about this. I agree with @kxcd. It will be confusing because it is not a withdrawal from the wallet's perspective - it's an incoming tx just like a masternode reward, incoming payment, etc. Maybe something like platform transfer or asset/credit pool distribution. If we used * transfer, it could also be used to show asset lock txs as well as unlocks. What do you guys think?

kxcd commented 1 month ago

Thanks Phez, I like Platform Transfer best. Though I also like asset/credit pool and then let the wallet show you the direction since we can both send into Platform and receive from Platform, ideally, the name is the same for both transactions, just the direction will be different. The good thing about Platform Transfer is it can be in or out, it's directionless.

PastaPastaPasta commented 1 month ago

I want @thephez's approval for terminology / UX. I think I generally agree with him and @kxcd that "withdrawal" is confusing

knst commented 1 month ago

general NACK on "withdrawal" terminology. Want utACK from @thephez / suggestions

current version has mentioning of withdrawal only in functional tests, Qt & rpc are updated already

PastaPastaPasta commented 1 month ago

general NACK on "withdrawal" terminology. Want utACK from @thephez / suggestions

current version has mentioning of withdrawal only in functional tests, Qt & rpc are updated already

Maybe add information about what the terms are then in description?

knst commented 1 month ago

@knst knst force-pushed the feat-withdrawals-qt branch from 70cbef9 to 3cd2115

there's extra fixes and improvements. Also PR description is updated and re-made a screenshot with "platform transfer" instead "withdrawal"

@knst knst force-pushed the feat-withdrawals-qt branch from 3cd2115 to 9748a04

rebased on the top of current develop

dashpay / dash