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

Banking features to concealwallet #281

Merged
merged 50 commits into from
Jun 8, 2022

Conversation

cartoon-face
Copy link
Contributor

@cartoon-face cartoon-face commented Feb 21, 2022

New commands

  • deposit <months> <amount> - Creates a HLTC by depositing $CCX
  • withdraw <deposit_id> - Withdraws banked $CCX
  • list_deposits - Lists all known deposits in this wallet
  • deposit_info <deposit_id> - Get information for specific deposit

Notes

User Notes

  • Do not try and deposit more coins than what you can afford to loose, these are new features to the wallet and it does need fully testing before merge, after merge, deposit all the coins to your hearts content.
  • deposit <months> <amount> + withdraw <deposit_id> will always use the minimum fee in the network consensus, currently set to 0.001 $CCX.
  • <deposit_id> can be found via the list_deposits command. <deposit_id> is used for withdraw <deposit_id> and deposit_info <deposit_id>.
  • Newly created deposits that have not been processed by the blockchain yet will display "Please wait." in the "Unlock Height" field.

Dev Notes

  • deposit() might look like it belongs to the TransferCommand struct but as only 2 args are needed (term + amount), with none being optional, we don't use this struct, assign the args and use them as is.
  • a637768 doesn't replace the base function, this is merely just a copied function but we don't use std::vector<DepositID>, only DepositID used in concealwallet's withdraw command.
  • Fixed output for cn::Deposit::height. Now it uses data from deposit requests using the current node height to provide the output.
  • Fixed output for cn::Deposit::unlockHeight. Previously, this would only return the term of the deposit because height would return 0. Now that height is fixed, that means this has also been fixed.

What's New After Testing?

  • list_deposits
    • No longer has optional block_height arg.
  • Fixed output for list_deposits to match list_transfers
    • Red = locked deposit.
    • Green = unlocked deposit.
    • Grey = spent deposit.
  • Fixed output for deposits not processed yet
    • When calling unlockHeight on a deposit that hasn't been processed through the blockchain yet, it'll return a silly amount. We see this silly amount as a deposit yet to be processed so the text will now display "Please wait.", once the block passes it should be updated with the correct unlockHeight.
  • While making a deposit using the deposit <months> <amount> command, the client will ask you to confirm the details showing you the information for the deposit that is about to be processed. User can press Y or N to confirm or decline the deposit.
  • balance will now also show your deposited balances (actual & locked).

@krypt0x krypt0x requested review from AxVultis and krypt0x February 21, 2022 20:32
@krypt0x krypt0x added the enhancement New feature or request label Feb 21, 2022
Return false instead of breaking
Fix spelling
Ive been sus of tests for a while now, thinking one every X times during tests it fails
Im absolutly sure that `confirm_deposit()` works as intended
This follows the same logic as the tests (TestWalletLegacy.cpp)
Maybe we should make this universal across Deposit and WalletLegacyTransaction
while(true) with a return true statement? Just to be on the safe side
cartoon-face and others added 9 commits March 12, 2022 18:01
Old output used to have a header to list each detail, now the output will list:
```
    --- Deposit Info ---
ID: <deposit_id>
Amount: <amount>
Interest: <interest>
Term: <term>
Status: <is_locked_bool>
Unlock Height: <unlock_height>
```

deposit details will either be `BRIGHT_RED` if locked or `BRIGHT_GREEN` if unlocked
Works on my machine
Fixes calling `cn::Deposit::unlockHeight` + `cn::Deposit::height`
* adds new file DepositHelper to feed infomation
* adds get_deposit(id) to iwalletlegacy so we can call deposit = get_deposit(id)
* add height and unlockheight to legacydeposit
* add notes to previous work
cartoon-face and others added 3 commits March 30, 2022 00:37
Adds saveDeposits and loadDeposits to wallet serializer. This copies wallet serializer v2 with slight modifications
@krypt0x
Copy link
Member

krypt0x commented Mar 30, 2022

Great job! We'll do a full review and merge it ASAP. Thank you.

* add height to deposit request, which uses current node height
* add legacy deposit info to serialize
* remove testing logic from deposit_info
* add makeCenteredString to common namespace, allows us to use it universally
This was apart of testing during the PR and it's safe to say we don't need this.
@cartoon-face
Copy link
Contributor Author

I've updated the description for this again, it should cover everything I've done. I've written user notes and dev notes as well as what's new after testing. Sorry this took so long, we've done some great work here and thank you those who tested this throughout the development.

cartoon-face and others added 11 commits April 6, 2022 21:11
* removes unused function from namespace
With the new changes, the height would only of been imported from `concealwallet`s deposit request. This keeps support for the GUI, `if t_height == 0` it hasn't came from simplewallet
* this was suggested by Ax, it seems to work as an alternative serialise new data
* also removes table format from balance
@krypt0x
Copy link
Member

krypt0x commented May 23, 2022

Hi guys, is there any update on this PR? ETA?

@AxVultis AxVultis changed the base branch from development to banking June 8, 2022 14:33
@AxVultis AxVultis merged commit 03188e3 into ConcealNetwork:banking Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants