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

[Core][GUI][RPC] Exchange Address #2895

Merged
merged 18 commits into from Feb 16, 2024

Conversation

Liquid369
Copy link
Member

@Liquid369 Liquid369 commented Jan 11, 2024

This PR aims to implement a new OP_CODE and new address type defined as OP_EXCHANGEADDR and EXCHANGE_ADDRESS respectively.
With the new year and new regulations upcoming, we have been requested to make things easier on the exchanges by introducing this new address type that will not allow private transactions to be input to this address nor coinstake/coinbase transactions.
Consensus checks implemented, we have some variance from the FIRO implementation you can reference https://github.com/firoorg/firo/pull/1356/files for the address encoding and decoding.
OP_CODE introduced is essentially a NOP at the start of the scriptpubkey
New introduced Prefixes are
EX for Exchange Mainnet
EXT for Exchange Testnet
EXT for Exchange Regtest
Originally I went for just an E prefix, but for clarity I followed FIRO for having some difference, if we want to go another direction, I am not particular either way anymore.

New RPC command getnewexchangeaddress accepts a label to insert into Contacts
We are not using existing keys to create the pair but generating new.

Copy link

@panleone panleone 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 not a complete review: just some things I noticed by scrolling through the code + some questions.

src/chainparams.cpp Outdated Show resolved Hide resolved
src/chainparams.cpp Outdated Show resolved Hide resolved
src/chainparams.cpp Outdated Show resolved Hide resolved
src/chainparams.cpp Outdated Show resolved Hide resolved
src/consensus/tx_verify.cpp Outdated Show resolved Hide resolved
src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
test/functional/test_framework/script.py Show resolved Hide resolved
src/addressbook.cpp Outdated Show resolved Hide resolved
src/consensus/tx_verify.cpp Outdated Show resolved Hide resolved
src/destination_io.h Outdated Show resolved Hide resolved
Define in wallet and rpc
Adjust destination wrapper typo
Add conversion operator
Update DecodeDestination
Updates Prefixes from E to EXM, EXT, EXR respectively
@Liquid369 Liquid369 force-pushed the 2024_exchange_addr branch 3 times, most recently from 6d6713c to a17b82e Compare January 17, 2024 17:57
Fix

review changes minus tests

Remove other struct

Missed fixes

Fix redefinition

Remove struct

Adjust decode length
@Liquid369 Liquid369 force-pushed the 2024_exchange_addr branch 3 times, most recently from cfee5a1 to 49a1e69 Compare January 19, 2024 00:48
lint

Lint

Lint
@Fuzzbawls Fuzzbawls added this to the 5.6.0 milestone Feb 6, 2024
Copy link
Member

@Duddino Duddino left a comment

Choose a reason for hiding this comment

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

Two very minor style nitpicks

src/destination_io.h Outdated Show resolved Hide resolved
src/key_io.cpp Outdated Show resolved Hide resolved
Copy link
Member

@Duddino Duddino left a comment

Choose a reason for hiding this comment

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

tACK 3743ce1

Maybe we could have a nicer error message when trying to send shield to an exchange address (currently it's Failed to accept tx in the memory pool (reason: bad-txns-invalid-sapling)), but I'm happy to merge as is.

Copy link
Member

@Duddino Duddino left a comment

Choose a reason for hiding this comment

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

Fix the qt build please

Adjust wallet model
@Liquid369
Copy link
Member Author

Fixed

Copy link
Member

@Duddino Duddino left a comment

Choose a reason for hiding this comment

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

tACK 1444336

Copy link

@panleone panleone left a comment

Choose a reason for hiding this comment

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

utACK 1444336

There is still some refactor to do, but it can be done in a different PR since there is not much time left for the release

@Fuzzbawls Fuzzbawls merged commit d293ad2 into PIVX-Project:master Feb 16, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants