Optional change address #4129

paweljakubas · 2023-09-20T15:39:18Z

Comments

Issue Number

HeinrichApfelmus

Thank you! 😊 I have finally found the time to take a first look.

As for the user-facing design, I'm a bit unsure whether a state flag oneChangeAddressMode is the best solution — one alternative design would be to allow the user to supply an explicit change address in the "balance transaction" endpoint. That said, while this alternative has the advantage of avoiding more state, I'm not sure about it either. 🤔 In the end, I agree the state flag is reasonable.

Concerning the implementation, I have three larger requests:

Instead of using Bool, could you use a more "speaking" enumeration type? I.e.

data SeqState … = { …
    , changeAddressMode :: ChangeAddressMode
    }

data ChangeAddressMode
    = SingleChangeAddress
    | IncreasingChangeAddresses

This makes the code easier to understand and maintain in the long run.

For the implementation of setOneChangeAddressMode and setOneChangeAddressModeShared, we are now in the excellent position that we can work with the WalletState data type directly — it captures the entire state of the wallet in an ordinary Haskell data type, the state is no longer implicit in the database.

I have taken care of this point and added a commit which rewrites these two functions as pure functions with a little glue code onWalletState.

For the database, please use the Migration.New facilities — the old way is dead; the new way is much more robust. You will need to implement a migration and add it to runNewStyleMigrations. Implementing a new Migration is straightforward, see for example migrateDelegations in

cardano-wallet/lib/wallet/src/Cardano/Wallet/DB/Store/Delegations/Migration.hs

Lines 94 to 110 in 6986d98

    
           migration 
        
               :: UpdateStore (SqlPersistT IO) (Operation SlotNo PoolId) 
        
               -> ReadDBHandle IO () 
        
           migration store = do 
        
               conn <- asks dbConn 
        
               r <- liftIO $ isFieldPresent conn $ DBField StakeKeyCertSlot 
        
               case r of 
        
                   TableMissing -> return () 
        
                   ColumnMissing -> return () 
        
                   ColumnPresent -> withReaderT dbBackend $ do 
        
                       old <- readOldEncoding 
        
                       writeS store old 
        
                       rawExecute "DROP TABLE stake_key_certificate"  [] 
        
                       rawExecute "DROP TABLE delegation_certificate"  [] 
        
           migrateDelegations :: Migration (ReadDBHandle IO) 2 3 
        
           migrateDelegations = mkMigration $ migration mkStoreDelegations

But other than that, I think this looks good.

HeinrichApfelmus

Thanks! 😊 I have a couple more requests:

With the "speaking" constructors for ChangeAddressMode, the name oneChangeAddresMode no longer makes sense — changeAddresMode is better. I had change it in many places, but not all of them — could you complete this?
A couple of requests on the use of the Migration.New facilities. In particular, could you move the module where the migration is define to the Checkpoints.Migration?

I'll take another look at the documentation after that.

HeinrichApfelmus · 2023-10-17T13:45:08Z

lib/address-derivation-discovery/lib/Cardano/Wallet/Address/Discovery.hs

+data ChangeAddressMode
+    = SingleChangeAddress
+    | IncreasingChangeAddresses
+    deriving stock (Generic, Show, Eq)


Suggested change

data ChangeAddressMode

= SingleChangeAddress

| IncreasingChangeAddresses

deriving stock (Generic, Show, Eq)

-- | How to generate change addresses.

data ChangeAddressMode

= SingleChangeAddress

-- ^ Use a single address for all change outputs.

| IncreasingChangeAddresses

-- ^ For every change output, increase a counter and derive an address from that.

deriving stock (Generic, Show, Eq)

HeinrichApfelmus · 2023-10-17T13:45:29Z

lib/address-derivation-discovery/lib/Cardano/Wallet/Address/Discovery/Sequential.hs

+    , oneChangeAddressMode :: ChangeAddressMode
+        -- ^ One change address mode. If switched on then next transactions will
+        -- use the same change address. If no then next change index for
+        -- each next transaction is used


Suggested change

, oneChangeAddressMode :: ChangeAddressMode

-- ^ One change address mode. If switched on then next transactions will

-- use the same change address. If no then next change index for

-- each next transaction is used

, changeAddressMode :: ChangeAddressMode

-- ^ How to generate change addresses.

HeinrichApfelmus · 2023-10-17T13:45:41Z

lib/address-derivation-discovery/lib/Cardano/Wallet/Address/Discovery/Sequential.hs

    { internalPool = newSeqAddressPool @n accXPub g
    , externalPool = newSeqAddressPool @n accXPub g
    , accountXPub = accXPub
    , policyXPub = policyXPubM
    , rewardAccountKey = rewardXPub
    , pendingChangeIxs = emptyPendingIxs
    , derivationPrefix = DerivationPrefix ( purpose, coinTypeAda, minBound )
+    , oneChangeAddressMode = change


Suggested change

, oneChangeAddressMode = change

, changeAddressMode = change

HeinrichApfelmus · 2023-10-17T13:48:26Z

lib/address-derivation-discovery/lib/Cardano/Wallet/Address/Discovery/Sequential.hs

+            if oneChangeAddressMode st == SingleChangeAddress  then
+                ( ixMin, updatePending (pendingChangeIxs st) )
+            else
+                nextChangeIndex (getPool $ internalPool st) (pendingChangeIxs st)


Suggested change

if oneChangeAddressMode st == SingleChangeAddress then

( ixMin, updatePending (pendingChangeIxs st) )

else

nextChangeIndex (getPool $ internalPool st) (pendingChangeIxs st)

case changeAddressMode st of

SingleChangeAddress ->

( ixMin, updatePending (pendingChangeIxs st) )

IncreasingChangeAddresses ->

nextChangeIndex (getPool $ internalPool st) (pendingChangeIxs st)

A case statement "speaks" more, making it clearer which case is supposed to do what. 🤓

HeinrichApfelmus · 2023-10-17T13:48:52Z

lib/address-derivation-discovery/lib/Cardano/Wallet/Address/Discovery/Shared.hs

+    , oneChangeAddressMode :: ChangeAddressMode
+        -- ^ One change address mode. If switched on then next transactions will
+        -- use the same change address. If no then next change index for
+        -- each next transaction is used


Suggested change

, oneChangeAddressMode :: ChangeAddressMode

-- ^ One change address mode. If switched on then next transactions will

-- use the same change address. If no then next change index for

-- each next transaction is used

, changeAddressMode :: ChangeAddressMode

-- ^ How to generate change addresses.

HeinrichApfelmus · 2023-10-17T14:01:42Z

lib/wallet/src/Cardano/Wallet/DB/Sqlite/Schema.hs

@@ -346,6 +347,7 @@ SeqState
    seqStatePolicyXPub        B8.ByteString Maybe  sql=policy_xpub
    seqStateRewardXPub        B8.ByteString        sql=reward_xpub
    seqStateDerivationPrefix  W.DerivationPrefix   sql=derivation_prefix
+    seqStateOneChangeAddrMode W.ChangeAddressMode  sql=change_addr_mode


Suggested change

seqStateOneChangeAddrMode W.ChangeAddressMode sql=change_addr_mode

seqStateChangeAddrMode W.ChangeAddressMode sql=change_addr_mode

HeinrichApfelmus · 2023-10-17T14:01:50Z

lib/wallet/src/Cardano/Wallet/DB/Sqlite/Schema.hs

@@ -433,6 +435,7 @@ SharedState
    sharedStateDelegationScript         (Script Cosigner) Maybe   sql=delegation_script
    sharedStateRewardAccount            W.RewardAccount Maybe     sql=reward_account
    sharedStateDerivationPrefix         W.DerivationPrefix        sql=derivation_prefix
+    sharedStateOneChangeAddrMode        W.ChangeAddressMode       sql=change_addr_mode


Suggested change

sharedStateOneChangeAddrMode W.ChangeAddressMode sql=change_addr_mode

sharedStateChangeAddrMode W.ChangeAddressMode sql=change_addr_mode

HeinrichApfelmus · 2023-10-17T14:03:44Z

lib/wallet/src/Cardano/Wallet/DB/Sqlite/Migration/Old.hs

+    -- | Adds an 'one_change_addr_mode' column to the 'seq_state'
+    -- table if it is missing.
+    --
+    addOneChangeAddrModeIfMissing :: Sqlite.Connection -> IO ()
+    addOneChangeAddrModeIfMissing conn = do
+        addColumn_ conn True (DBField SeqStateOneChangeAddrMode) value
+      where
+        value = "FALSE"
+


Suggested change

-- | Adds an 'one_change_addr_mode' column to the 'seq_state'

-- table if it is missing.

--

addOneChangeAddrModeIfMissing :: Sqlite.Connection -> IO ()

addOneChangeAddrModeIfMissing conn = do

addColumn_ conn True (DBField SeqStateOneChangeAddrMode) value

where

value = "FALSE"

This goes away now.

HeinrichApfelmus · 2023-10-17T14:03:58Z

lib/wallet/src/Cardano/Wallet/DB/Sqlite/Migration/Old.hs

@@ -170,6 +170,7 @@ migrateManually tr key defaultFieldValues =
    , moveRndUnusedAddresses
    , cleanupSeqStateTable
    , addPolicyXPubIfMissing
+    , addOneChangeAddrModeIfMissing


Suggested change

, addOneChangeAddrModeIfMissing

HeinrichApfelmus · 2023-10-17T14:06:24Z

lib/wallet/src/Cardano/Wallet/DB/Store/WalletState/Migration.hs

Could you please move this module to Cardano.Wallet.DB.Store.Checkpoints.Migration? That puts it closer to the thing that is being migrated (i.e. the address state).

corrections

… testing

…nts the work accomplished in this PR. Before you submit, don't forget to: CODE-OF-CONDUCT.md CONTRIBUTING.md LICENSE MAINTAINERS.md README.md cabal.project default.nix docker-compose.yml docs flake.lock flake.nix floskell.json fourmolu.yaml hie-direnv.yaml justfile lib nix prototypes reports scripts shell.nix specifications test touch.me.CI weeder.dhall Make sure the GitHub PR fields are correct: ✓ Set a good Title for your PR. ✓ Assign yourself to the PR. ✓ Assign one or more reviewer(s). ✓ Link to a Jira issue, and/or other GitHub issues or PRs. ✓ In the PR description delete any empty sections and all text commented in  - [x] adjust post wallet api and enabling one change addr flag - [x] extend SeqState - [x] adjust nextChangeIndex - [x] update db schema and related code - [x] handle db migration - [x] add integration test proving the case - [x] allow wallet to go from multi-change-address to one-change-address mode and vice versa (update wallet api change) - [x] show the case in integration testing - [x] add spec - [x] add PUT for shared wallets - [x] do the same like for shelley for shared wallet ### Comments  ### Issue Number ADP-3145 Source commit: e5a6ca6

paweljakubas self-assigned this Sep 20, 2023

paweljakubas force-pushed the paweljakubas/adp-3145/optional-change-address branch 3 times, most recently from 0312415 to a076070 Compare September 27, 2023 16:11

paweljakubas marked this pull request as ready for review September 29, 2023 16:05

paweljakubas requested review from HeinrichApfelmus, jonathanknowles and Anviking September 29, 2023 16:05

paweljakubas force-pushed the paweljakubas/adp-3145/optional-change-address branch 3 times, most recently from 4a69010 to 9c88c24 Compare October 4, 2023 14:04

HeinrichApfelmus reviewed Oct 9, 2023

View reviewed changes

paweljakubas force-pushed the paweljakubas/adp-3145/optional-change-address branch 3 times, most recently from ff7a0be to e3481ec Compare October 13, 2023 14:44

HeinrichApfelmus reviewed Oct 17, 2023

View reviewed changes

paweljakubas force-pushed the paweljakubas/adp-3145/optional-change-address branch 3 times, most recently from 127105d to 42de1ff Compare October 19, 2023 15:46

paweljakubas requested a review from HeinrichApfelmus October 19, 2023 15:46

paweljakubas force-pushed the paweljakubas/adp-3145/optional-change-address branch from 42de1ff to 2229d7d Compare October 20, 2023 14:56

paweljakubas force-pushed the paweljakubas/adp-3145/optional-change-address branch from fd8163f to 3db4946 Compare October 30, 2023 08:35

HeinrichApfelmus force-pushed the paweljakubas/adp-3145/optional-change-address branch 8 times, most recently from 05e5e67 to 0937343 Compare November 17, 2023 11:58

paweljakubas and others added 24 commits November 22, 2023 10:13

integration test - part 2

69fc31b

integration test - part 3

4313d91

change PUT wallet api plus add spec

57ea7ef

add setOneChangeAddressMode

53c05b3

use setOneChangeAddressMode

3e052a3

put wallet data integration test

9af3133

swagger plus api adjustments for shared

afad93f

extend SharedState

77d148a

integration test fix

0dc7083

corrections

define setOneChangeAddressModeShared

29fb64b

use setOneChangeAddressModeShared

d06d042

integration testfor shared - part 1

d11e834

integration testfor shared - part 2

ccd9d86

integration testfor shared - part 3 plus genChange fix

319998c

add PUT metadata for shared wallets and check it works in integration…

93def7a

… testing

integration test for shared - part 4

278d5dd

introduce ChangeAddressMode

9526860

resolve ApiWalletPut and ApiWalletPutExtended

861242f

introduce ChangeAddressMode in DB

698504b

Export noManualMigration and noAutoMigrations

9ca53f5

Export readWalletId

c502c46

new prologue migration

635c337

Checkpoints.MigrationSpec

8dc840d

small fixes

288700e

paweljakubas force-pushed the paweljakubas/adp-3145/optional-change-address branch from f161566 to 288700e Compare November 22, 2023 09:13

paweljakubas added this pull request to the merge queue Nov 22, 2023

Merged via the queue into master with commit e5a6ca6 Nov 22, 2023
2 checks passed

paweljakubas deleted the paweljakubas/adp-3145/optional-change-address branch November 22, 2023 10:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optional change address #4129

Optional change address #4129

paweljakubas commented Sep 20, 2023 •

edited by HeinrichApfelmus

Loading

HeinrichApfelmus left a comment

HeinrichApfelmus left a comment

HeinrichApfelmus Oct 17, 2023

paweljakubas Oct 18, 2023

HeinrichApfelmus Oct 17, 2023

paweljakubas Oct 18, 2023

HeinrichApfelmus Oct 17, 2023

paweljakubas Oct 18, 2023

HeinrichApfelmus Oct 17, 2023

paweljakubas Oct 18, 2023

HeinrichApfelmus Oct 17, 2023

paweljakubas Oct 18, 2023

HeinrichApfelmus Oct 17, 2023

paweljakubas Oct 18, 2023

HeinrichApfelmus Oct 17, 2023

paweljakubas Oct 18, 2023

HeinrichApfelmus Oct 17, 2023

HeinrichApfelmus Oct 17, 2023

paweljakubas Oct 18, 2023

HeinrichApfelmus Oct 17, 2023

paweljakubas Oct 18, 2023

	migration
	:: UpdateStore (SqlPersistT IO) (Operation SlotNo PoolId)
	-> ReadDBHandle IO ()
	migration store = do
	conn <- asks dbConn
	r <- liftIO $ isFieldPresent conn $ DBField StakeKeyCertSlot
	case r of
	TableMissing -> return ()
	ColumnMissing -> return ()
	ColumnPresent -> withReaderT dbBackend $ do
	old <- readOldEncoding
	writeS store old
	rawExecute "DROP TABLE stake_key_certificate" []
	rawExecute "DROP TABLE delegation_certificate" []

	migrateDelegations :: Migration (ReadDBHandle IO) 2 3
	migrateDelegations = mkMigration $ migration mkStoreDelegations

	seqStateOneChangeAddrMode W.ChangeAddressMode sql=change_addr_mode
	seqStateChangeAddrMode W.ChangeAddressMode sql=change_addr_mode

	sharedStateOneChangeAddrMode W.ChangeAddressMode sql=change_addr_mode
	sharedStateChangeAddrMode W.ChangeAddressMode sql=change_addr_mode

Optional change address #4129

Optional change address #4129

Conversation

paweljakubas commented Sep 20, 2023 • edited by HeinrichApfelmus Loading

Comments

Issue Number

HeinrichApfelmus left a comment

Choose a reason for hiding this comment

HeinrichApfelmus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paweljakubas commented Sep 20, 2023 •

edited by HeinrichApfelmus

Loading