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

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

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

knst
Copy link
Collaborator

@knst knst commented Jul 19, 2024

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 withdrawal transaction on RegTest and check it in Dash Core

image

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 #6128

Checklist:

  • 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
  • I have assigned this pull request to a milestone

@knst knst added RPC Some notable changes to RPC params/behaviour/descriptions backport-candidate-21.0.x labels Jul 19, 2024
@knst knst added this to the 21 milestone Jul 19, 2024
@knst knst force-pushed the feat-withdrawals-qt branch 2 times, most recently from fbd50cf to b9118f5 Compare July 19, 2024 14:10
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

b9118f5 looks incomplete - you just import a key but you don't check any tx/rpc

@kxcd
Copy link

kxcd commented Jul 20, 2024

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
Copy link
Collaborator Author

knst commented Jul 20, 2024

@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.

Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

General NACK for v21.0; we are in feature freeze and releasing in 9 days. We need to avoid new features at all costs. We can include this in a fast following 21.1

@thephez
Copy link
Collaborator

thephez commented Jul 20, 2024

@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?

@knst knst modified the milestones: 21, 21.1 Jul 20, 2024
@kxcd
Copy link

kxcd commented Jul 22, 2024

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.

@knst knst changed the title feat: make a support of Qt app to show withdrawal transaction feat: make a support of Qt app to show Platform transfer Tx Jul 22, 2024
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

utACK 70cbef9 (mac-build CI failure is unrelated)

@PastaPastaPasta
Copy link
Member

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

Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

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

@knst
Copy link
Collaborator Author

knst commented Aug 2, 2024

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
Copy link
Member

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?

@@ -408,6 +408,7 @@ struct WalletTx
int64_t time;
std::map<std::string, std::string> value_map;
bool is_coinbase;
bool is_platform_transfer;
Copy link
Member

Choose a reason for hiding this comment

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

I know none of the others do it, but we should ensure these are always initialized

Suggested change
bool is_platform_transfer;
bool is_platform_transfer{false};

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RPC Some notable changes to RPC params/behaviour/descriptions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants