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

Shared wallets delegation #3830

Merged
merged 30 commits into from
Apr 17, 2023

Conversation

paweljakubas
Copy link
Contributor

@paweljakubas paweljakubas commented Apr 4, 2023

  • sneak delegation action to ctxTx
  • refactor constraints in constructSharedTx
  • introduce ErrConstructTxStakingInvalid with integration test
  • create proper staking credential during tx construction
  • introduce script as credential in shelley module
  • introduce txStakingTemplate in TxSkeleton
  • fee adjustment
  • inject delegation script if it is valid
  • add emptySharedWalletDelegating and fixtureSharedWalletdelegating
  • constructTx with delegation integration test
  • account deposit/refunds
  • make sure decodeSharedTransaction account deposits/refunds and certificates properly
  • extending WitnessCtx in the context of decodeSharedTransaction
  • signing mechanism
  • show signing in integration testing
  • sneaking WitnessCtx in signTransaction
  • guarding submission and adding proper integration tests

Comments

Issue Number

adp-2602

@paweljakubas paweljakubas self-assigned this Apr 4, 2023
@paweljakubas paweljakubas force-pushed the paweljakubas/adp-2602/shared-wallets-delegation branch from 7096366 to 18c509b Compare April 6, 2023 08:01
@@ -491,8 +502,10 @@ signTransaction
-> Cardano.Tx era
signTransaction
networkId
witCountCtx
Copy link
Member

Choose a reason for hiding this comment

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

Can we not just extract the key hashes from all scripts, and try to sign with all script signers?

Comment on lines +1233 to +1221
estimateDelegSigningKeys = \case
Cardano.StakeAddressRegistrationCertificate _ -> 0
Cardano.StakeAddressDeregistrationCertificate cred ->
estimateWitNumForCred cred
Cardano.StakeAddressDelegationCertificate cred _ ->
estimateWitNumForCred cred
_ -> 1
Copy link
Member

Choose a reason for hiding this comment

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

Ah perfect, nice 👍

Copy link
Member

@Anviking Anviking Apr 6, 2023

Choose a reason for hiding this comment

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

Or... no wait, the script credential case should be redundant with

scriptVkWitsUpperBound =
             fromIntegral
             $ sumVia estimateMaxWitnessRequiredPerInput
            $ mapMaybe toTimelockScript scripts

above, which is counting the keys needed for all timelock scripts.

So I think if we just replace the estimateWitNumForCred cred with 0 in estimateDelegSigningKeys, it should be better. I.e. it's only the certificates with key credentials we need to count separately.

Then you shouldn't need the partial estimateWitNumForCred either.

Copy link
Member

Choose a reason for hiding this comment

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

I tried doing this: try-fixing-estimateKeyWitCount.patch

Unfortunately we then run into this error:

  integration/src/Test/Integration/Scenario/API/Shared/Transactions.hs:1923:15:
  1) API Specifications, SHARED_TRANSACTIONS, SHARED_TRANSACTIONS_DELEGATION_01a - Can join stakepool, rejoin another and quit
       From the following response: Left
           ( ClientError
               ( Object
                   ( fromList
                       [
                           ( "code"
                           , String "created_invalid_transaction"
                           )
                       ,
                           ( "message"
                           , String "The submitted transaction was rejected by the local node. Here's an error message that may help with debugging: HardForkApplyTxErrFromEra S (S (S (S (S (Z (WrapApplyTxErr {unwrapApplyTxErr = ApplyTxError [UtxowFailure (UtxoFailure (AlonzoInBabbageUtxoPredFailure (FeeTooSmallUTxO (Coin 176900) (Coin 166800))))]})))))) Here is the tx: ShelleyTx ShelleyBasedEraBabbage     ( AlonzoTx         { body = TxBodyConstr TxBodyRaw             { _spendInputs = fromList                 [ TxIn                     ( TxId                         { _unTxId = SafeHash "2a96945fbefe340d7039095914ad82a805e9470dd2c9efbc900a89d0f40d2989" }                     )                     ( TxIx 0 )                 ]             , _collateralInputs = fromList []             , _referenceInputs = fromList []             , _outputs = StrictSeq                 { fromStrict = fromList                     [ Sized                         { sizedValue =                             ( Addr Mainnet                                 ( ScriptHashObj                                     ( ScriptHash "0270c7a715297905cac542c23c579bc126427e9fba734dddf6f63cdd" )                                 )                                 ( StakeRefBase                                     ( ScriptHashObj                                         ( ScriptHash "ff1086611634f68a35297e4d136400e18bd42f8ce3af4e337396a9bf" )                                     )                                 )                             , MaryValue 99998833200                                 ( fromList [] )                             , NoDatum                             , SNothing                             )                         , sizedSize = 71                         }                     ]                 }             , _collateralReturn = SNothing             , _totalCollateral = SNothing             , _certs = StrictSeq                 { fromStrict = fromList                     [ DCertDeleg                         ( RegKey                             ( ScriptHashObj                                 ( ScriptHash "ff1086611634f68a35297e4d136400e18bd42f8ce3af4e337396a9bf" )                             )                         )                     , DCertDeleg                         ( Delegate                             ( Delegation                                 { _delegator = ScriptHashObj                                     ( ScriptHash "ff1086611634f68a35297e4d136400e18bd42f8ce3af4e337396a9bf" )                                 , _delegatee = KeyHash "1b3dc19c6ab89eaffc8501f375bb03c11bf8ed5d183736b1d80413d6"                                 }                             )                         )                     ]                 }             , _wdrls = Wdrl                 { unWdrl = fromList [] }             , _txfee = Coin 166800             , _vldt = ValidityInterval                 { invalidBefore = SJust                     ( SlotNo 0 )                 , invalidHereafter = SJust                     ( SlotNo 36060 )                 }             , _update = SNothing             , _reqSignerHashes = fromList []             , _mint = MaryValue 0                 ( fromList [] )             , _scriptIntegrityHash = SNothing             , _adHash = SNothing             , _txnetworkid = SNothing             }         , wits = TxWitnessRaw             { _txwitsVKey = fromList                 [ WitVKeyInternal                     { wvkKey = VKey                         ( VerKeyEd25519DSIGN "1cf5424743e3a0336f230cd276be03a3de9200bdb273b722022fd193f1e9895d" )                     , wvkSig = SignedDSIGN                         ( SigEd25519DSIGN "273bfd43373979413453a9dfca0a8d0c2f634249a21291067af9e67603c25b860cc00ab455dc34433d8ee26a520ec25713d69d9fd218eaae1fc34c1bd7a5440f" )                     , wvkKeyHash = KeyHash "2f9ff4f1f66badab65790cb682c883db544bb738b0f60f0a91d6a715"                     , wvkBytes = "\x82X \x1cõBGCã 3o#\xcÒv¾\x3£Þ\x92\x0½²s·"\x2/Ñ\x93ñé\x89]X@';ýC79yA4S©ßÊ                       \x8d\xc/cBI¢\x12\x91\x6zùæv\x3Â[\x86\xcÀ                       ´UÜ4C=\x8eâjR\xeÂW\x13Ö\x9d\x9fÒ\x18ê®\x1fÃL\x1b×¥D\xf"                     }                 , WitVKeyInternal                     { wvkKey = VKey                         ( VerKeyEd25519DSIGN "68e6d1a3c63cbd8db90904d37282517d989784ddbec41b9147ef7f53fff3fa0f" )                     , wvkSig = SignedDSIGN                         ( SigEd25519DSIGN "859290a3661d213ab3637ec906e8664ff23be14df0add71ebca68330f5a2beb993a97c23227cf1878d5cbca271d314e46d11a84212887f09257abb66b7e9cd0d" )                     , wvkKeyHash = KeyHash "48073afca7453a4862c76fe52c06510a26a4c29fb216efac6c1e1606"                     , wvkBytes = "\x82X hæÑ£Æ<½\x8d¹\x9\x4Ór\x82Q}\x98\x97\x84ݾÄ\x1b\x91Gï\x7fSÿóú\xfX@\x85\x92\x90£f\x1d!:³c~É\x6èfOò;áMð\xad×\x1e¼¦\x830õ¢¾¹\x93©|#"|ñ\x87\x8d\¼¢qÓ\x14äm\x11¨B\x12\x88\x7f\x9%z»f·éÍ\xd"                     }                 , WitVKeyInternal                     { wvkKey = VKey                         ( VerKeyEd25519DSIGN "cbaca08d7c8f8e62ec718d71f75626f253eb4932870fc66619433fa99265f0f9" )                     , wvkSig = SignedDSIGN                         ( SigEd25519DSIGN "31b05cc31d43b53d0a1981bd08b6a5da2a3b2b212b8b82305705de2b3a6b30d63ab0aef95927cb7b3b65e95ce19110b61d5d0dca5069d48e10fa117764a3a400" )                     , wvkKeyHash = KeyHash "62d7f6c4fd8b483841fa344008bad7c7c835eb9559c97b5c4ed91ce2"                     , wvkBytes = "\x82X ˬ \x8d|\x8f\x8ebìq\x8dq÷V&òSëI2\x87\xfÆf\x19C?©\x92eðùX@1°\Ã\x1dCµ=                       \x19\x81½\x8¶¥Ú*;+!+\x8b\x820W\x5Þ+:k0Ö:°®ùY'Ë{;eé\á\x91\x10¶\x1d]\xdÊPiÔ\x8e\x10ú\x11wd£¤\x0"                     }                 , WitVKeyInternal                     { wvkKey = VKey                         ( VerKeyEd25519DSIGN "b4e9a2469d41a6cf4bd70ab62c4b5f99b4512eec5cc9560d005e9f154ede0336" )                     , wvkSig = SignedDSIGN                         ( SigEd25519DSIGN "226e763ae879ed25b712056c025ad8dd22b8476939d2516a7e7c6ab981ab63b7857f95d7b80fb2a71efc35377cf8981db4ed6bea956d6f1e1697f76c46e81a03" )                     , wvkKeyHash = KeyHash "dda549acce7d5301739214cd871f15dd6efb37dc19fdb18fa723f181"                     , wvkBytes = "\x82X ´é¢F\x9dA¦ÏK×                       ¶,K_\x99´Q.ì\ÉV\xd\x0^\x9f\x15NÞ\x36X@"nv:èyí%·\x12\x5l\x2ZØÝ"¸Gi9ÒQj~|j¹\x81«c·\x85\x7f\x95׸\xf²§\x1eü57|ø\x98\x1d´íkê\x95mo\x1e\x16\x97÷lFè\x1a\x3"                     }                 ]             , _txwitsBoot = fromList []             , _txscripts = fromList                 [                     ( ScriptHash "e1b97e195443fb0b73ffd08ed0ec05de957e61a0a8af7004cff324c0"                     , TimelockScript TimelockConstr AnyOf                         ( StrictSeq                             { fromStrict = fromList                                 [ TimelockConstr Signature                                     ( KeyHash "62d7f6c4fd8b483841fa344008bad7c7c835eb9559c97b5c4ed91ce2" )                                 , TimelockConstr Signature                                     ( KeyHash "dda549acce7d5301739214cd871f15dd6efb37dc19fdb18fa723f181" )                                 ]                             }                         )                     )                 ,                     ( ScriptHash "ff1086611634f68a35297e4d136400e18bd42f8ce3af4e337396a9bf"                     , TimelockScript TimelockConstr AllOf                         ( StrictSeq                             { fromStrict = fromList                                 [ TimelockConstr Signature                                     ( KeyHash "2f9ff4f1f66badab65790cb682c883db544bb738b0f60f0a91d6a715" )                                 , TimelockConstr Signature                                     ( KeyHash "48073afca7453a4862c76fe52c06510a26a4c29fb216efac6c1e1606" )                                 ]                             }                         )                     )                 ]             , _txdats = TxDatsRaw                 ( fromList [] )             , _txrdmrs = RedeemersRaw                 ( fromList [] )             }         , isValid = IsValid True         , auxiliaryData = SNothing         }     )"
                           )
                       ]
                   )
               )
           )
       While verifying value:
         ( Status
             { statusCode = 500
             , statusMessage = "Internal Server Error"
             }
         , Left
             ( ClientError
                 ( Object
                     ( fromList
                         [
                             ( "code"
                             , String "created_invalid_transaction"
                             )
                         ,
                             ( "message"
                             , String "The submitted transaction was rejected by the local node. Here's an error message that may help with debugging: HardForkApplyTxErrFromEra S (S (S (S (S (Z (WrapApplyTxErr {unwrapApplyTxErr = ApplyTxError [UtxowFailure (UtxoFailure (AlonzoInBabbageUtxoPredFailure (FeeTooSmallUTxO (Coin 176900) (Coin 166800))))]})))))) Here is the tx: ShelleyTx ShelleyBasedEraBabbage     ( AlonzoTx         { body = TxBodyConstr TxBodyRaw             { _spendInputs = fromList                 [ TxIn                     ( TxId                         { _unTxId = SafeHash "2a96945fbefe340d7039095914ad82a805e9470dd2c9efbc900a89d0f40d2989" }                     )                     ( TxIx 0 )                 ]             , _collateralInputs = fromList []             , _referenceInputs = fromList []             , _outputs = StrictSeq                 { fromStrict = fromList                     [ Sized                         { sizedValue =                             ( Addr Mainnet                                 ( ScriptHashObj                                     ( ScriptHash "0270c7a715297905cac542c23c579bc126427e9fba734dddf6f63cdd" )                                 )                                 ( StakeRefBase                                     ( ScriptHashObj                                         ( ScriptHash "ff1086611634f68a35297e4d136400e18bd42f8ce3af4e337396a9bf" )                                     )                                 )                             , MaryValue 99998833200                                 ( fromList [] )                             , NoDatum                             , SNothing                             )                         , sizedSize = 71                         }                     ]                 }             , _collateralReturn = SNothing             , _totalCollateral = SNothing             , _certs = StrictSeq
          { fromStrict = fromList                     [ DCertDeleg                         ( RegKey                             ( ScriptHashObj                                 ( ScriptHash "ff1086611634f68a35297e4d136400e18bd42f8ce3af4e337396a9bf" )                             )                         )                     , DCertDeleg                         ( Delegate                             ( Delegation                                 { _delegator = ScriptHashObj                                     ( ScriptHash "ff1086611634f68a35297e4d136400e18bd42f8ce3af4e337396a9bf" )                                 , _delegatee = KeyHash "
1b3dc19c6ab89eaffc8501f375bb03c11bf8ed5d183736b1d80413d6"                                 }                             )                         )                     ]                 }             , _wdrls = Wdrl                 { unWdrl = fromList [] }             , _txfee = Coin 166800             , _vldt = ValidityInterval                 { invalidBefore = SJust                     ( SlotNo 0 )                 , invalidHereafter = SJust                     ( SlotNo 36060 )                 }             , _update = SNothing             , _reqSignerHashes = fromList []             , _mint = MaryValue 0                 ( fromList [
] )             , _scriptIntegrityHash = SNothing             , _adHash = SNothing             , _txnetworkid = SNothing             }         , wits = TxWitnessRaw             { _txwitsVKey = fromList                 [ WitVKeyInternal                     { wvkKey = VKey                         ( VerKeyEd25519DSIGN "1cf5424743e3a0336f230cd276be03a3de9200bdb273b722022fd193f1e9895d" )                     , wvkSig = SignedDSIGN                         ( SigEd25519DSIGN "273bfd43373979413453a9dfca0a8d0c2f634249a21291067af9e67603c25b860cc00ab455dc34433d8ee26a520ec25713d69d9fd218eaae1fc34c1bd7a5440f" )                     , wvkKeyHash = Key
Hash "2f9ff4f1f66badab65790cb682c883db544bb738b0f60f0a91d6a715"                     , wvkBytes = "\x82X \x1cõBGCã 3o#\xcÒv¾\x3£Þ\x92\x0½²s·"\x2/Ñ\x93ñé\x89]X@';ýC79yA4S©ßÊ                       \x8d\xc/cBI¢\x12\x91\x6zùæv\x3Â[\x86\xcÀ                       ´UÜ4C=\x8eâjR\xeÂW\x13Ö\x9d\x9fÒ\x18ê®\x1fÃL\x1b×¥D\xf"                     }                 , WitVKeyInternal                     { wvkKey = VKey                         ( VerKeyEd25519DSIGN "68e6d1a3c63cbd8db90904d37282517d989784ddbec41b9147ef7f53fff3fa0f" )                     , wvkSig = SignedDSIGN                         ( SigEd25519DSIGN "859290a3661d213ab3637ec906e8664ff23be
14df0add71ebca68330f5a2beb993a97c23227cf1878d5cbca271d314e46d11a84212887f09257abb66b7e9cd0d" )                     , wvkKeyHash = KeyHash "48073afca7453a4862c76fe52c06510a26a4c29fb216efac6c1e1606"                     , wvkBytes = "\x82X hæÑ£Æ<½\x8d¹\x9\x4Ór\x82Q}\x98\x97\x84ݾÄ\x1b\x91Gï\x7fSÿóú\xfX@\x85\x92\x90£f\x1d!:³c~É\x6èfOò;áMð\xad×\x1e¼¦\x830õ¢¾¹\x93©|#"\x87\x8d\¼¢qÓ\x14äm\x11¨B\x12\x88\x7f\x9%z»f·éÍ\xd"                     }                 , WitVKeyInternal                     { wvkKey = VKey                         ( VerKeyEd25519DSIGN "cbaca08d7c8f8e62ec718d71f75626f253eb4932870fc66619433fa99265f0f9" )
  , wvkSig = SignedDSIGN                         ( SigEd25519DSIGN "31b05cc31d43b53d0a1981bd08b6a5da2a3b2b212b8b82305705de2b3a6b30d63ab0aef95927cb7b3b65e95ce19110b61d5d0dca5069d48e10fa117764a3a400" )                     , wvkKeyHash = KeyHash "62d7f6c4fd8b483841fa344008bad7c7c835eb9559c97b5c4ed91ce2"                     , wvkBytes = "\x82X ˬ \x8d|\x8f\x8ebìq\x8dq÷V&òSëI2\x87\xfÆf\x19C\x92eðùX@1°\Ã\x1dCµ=                       \x19\x81½\x8¶¥Ú*;+!+\x8b\x820W\x5Þ+:k0Ö:°®ùY'Ë{;eé\á\x91\x10\x1d]\xdÊPiÔ\x8e\x10ú\x11wd£¤\x0"                     }                 , WitVKeyInternal                     { wvkKey = VKey
  ( VerKeyEd25519DSIGN "b4e9a2469d41a6cf4bd70ab62c4b5f99b4512eec5cc9560d005e9f154ede0336" )                     , wvkSig = SignedDSIGN                         ( SigEd25519DSIGN "226e763ae879ed25b712056c025ad8dd22b8476939d2516a7e7c6ab981ab63b7857f95d7b80fb2a71efc35377cf8981db4ed6bea956d6f1e1697f76c46e81a03" )                     , wvkKeyHash = KeyHash "dda549acce7d5301739214cd871f15dd6efb37dc19fdb18fa723f181"                     , wvkBytes = "\x82X ´é¢F\x9dA¦ÏK×                       ¶,K_\x99´Q.ì\ÉV\xd\x0^\x9f\x15NÞ\x36X@"nv:èyí%·\x12\x5l\x2ZØÝ"¸Gi9ÒQj~|j¹\x81«c·\x85\x7f\x95׸\xf²§\x1eü57|ø\x98\x1d´íkê\x95mo\x1e\x16\x97÷lFè\x1a\x3"                     }                 ]             , _txwitsBoot = fromList []             , _txscripts = fromList                 [                     ( ScriptHash "e1b97e195443fb0b73ffd08ed0ec05de957e61a0a8af7004cff324c0"                     , TimelockScript TimelockConstr AnyOf                         ( StrictSeq                             { fromStrict = fromList                                 [ TimelockConstr Signature                                     ( KeyHash "62d7f6c4fd8b483841fa344008bad7c7c835eb9559c97b5c4ed91ce2" )                                 , TimelockConstr Signature                                     ( KeyHash "dda549acce7d5301739214cd871f15dd6efb37dc19fdb18fa723f181" )                                 ]                             }                         )                     )                 ,                     ( ScriptHash "ff1086611634f68a35297e4d136400e18bd42f8ce3af4e337396a9bf"                     , TimelockScript TimelockConstr AllOf                         ( StrictSeq                             { fromStrict = fromList                                 [ TimelockConstr Signature                                     ( KeyHash "2f9ff4f1f66badab65790cb682c883db544bb738b0f60f0a91d6a715" )                                 , TimelockConstr Signature                                     ( KeyHash "48073afca7453a4862c76fe52c06510a26a4c29fb216efac6c1e1606" )                                 ]                             }                         )                     )                 ]             , _txdats = TxDatsRaw                 ( fromList [] )             , _txrdmrs = RedeemersRaw                 ( fromList [] )             }         , isValid = IsValid True         , auxiliaryData = SNothing         }     )"
                             )
                         ]
                     )
                 )
             )
         )
       expected: Status {
                   statusCode = 202,
                   statusMessage = "Accepted"
                 }
        but got: Status {
                   statusCode = 500,
                   statusMessage = "Internal Server Error"
                 }

Copy link
Member

Choose a reason for hiding this comment

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

We have delegation_template = All [a, b], payment_template = Any [a, b], and an estimated key witness count of 3... but 4 witnesses added!

I think in this case the 3 is correct (or at least using "optimal" reasoning).

It then sounds as if signTx for the second wallet doesn't realise the input doesn't need to be signed more, and adds a witness anyway... but shouldn't this kind of failure have appeared before in that case? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, in the case we adopted for integration testing, we need (party1 and party2) wits from payment template and only one wit from delegation template. It is checked in integration testing when inspecting WitnessCount. Although superfluous the current solution works ...

Copy link
Member

@Anviking Anviking Apr 12, 2023

Choose a reason for hiding this comment

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

Although superfluous the current solution works ...

Yes, the test does pass as it is. But if this is because the 1 wit/cert overestimation makes up for the difference between estimateMaxWitnessRequiredPerInput and signing all key hashes in the script, then it wouldn't work for bigger script templates — e.g. Any [a, b, c]? (could be confirmed)

Possibly a better workaround would be to make the Any case here the same as the All case
https://github.com/input-output-hk/cardano-wallet/blob/206371ed53a2a9e1e64ba1d84624169431417c5b/lib/wallet/src/Cardano/Wallet/Primitive/AddressDiscovery/Shared.hs#L786
with a comment that it can be changed back, but only if we make signTx smarter (I think something similar would need to happen for the SomeOf)

@paweljakubas paweljakubas mentioned this pull request Apr 7, 2023
6 tasks
@paweljakubas paweljakubas force-pushed the paweljakubas/adp-2602/shared-wallets-delegation branch 2 times, most recently from b3c44c7 to 6cde360 Compare April 12, 2023 11:14
Anviking added a commit that referenced this pull request Apr 12, 2023
Main idea is that we're taking the sum of
1. Key witness count needed for key credentials
2. Key witness count needed for (native) scripts

Unfortunately, this commit also breaks an integration test: #3830 (comment)
as `signTransaction` adds too many witnesses — or at least more than
what is needed and more than what we made room for when balancing. Next
commit will fix this by "making more room" by increasing the estimated
`KeyWitnessCount` in a more proper way.
fixtureSharedWalletDelegating ctx = do
(walSharedA@(ApiSharedWallet (Right walA)), ApiSharedWallet (Right walB)) <-
emptySharedWalletDelegating ctx
fundSharedWallet @n ctx faucetUtxoAmt (NE.fromList [walSharedA])
Copy link
Member

@Anviking Anviking Apr 12, 2023

Choose a reason for hiding this comment

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

FYI for future: It would be faster to make the wallets funded already in the genesis file, rather than through txs

@Anviking Anviking self-requested a review April 12, 2023 15:03
paweljakubas pushed a commit that referenced this pull request Apr 13, 2023
Main idea is that we're taking the sum of
1. Key witness count needed for key credentials
2. Key witness count needed for (native) scripts

Unfortunately, this commit also breaks an integration test: #3830 (comment)
as `signTransaction` adds too many witnesses — or at least more than
what is needed and more than what we made room for when balancing. Next
commit will fix this by "making more room" by increasing the estimated
`KeyWitnessCount` in a more proper way.
@paweljakubas paweljakubas force-pushed the paweljakubas/adp-2602/shared-wallets-delegation branch from 68fe066 to 5c4c42a Compare April 13, 2023 07:07
paweljakubas pushed a commit that referenced this pull request Apr 13, 2023
Main idea is that we're taking the sum of
1. Key witness count needed for key credentials
2. Key witness count needed for (native) scripts

Unfortunately, this commit also breaks an integration test: #3830 (comment)
as `signTransaction` adds too many witnesses — or at least more than
what is needed and more than what we made room for when balancing. Next
commit will fix this by "making more room" by increasing the estimated
`KeyWitnessCount` in a more proper way.
@paweljakubas paweljakubas force-pushed the paweljakubas/adp-2602/shared-wallets-delegation branch from 5c4c42a to ebc124a Compare April 13, 2023 10:45
paweljakubas pushed a commit that referenced this pull request Apr 14, 2023
Main idea is that we're taking the sum of
1. Key witness count needed for key credentials
2. Key witness count needed for (native) scripts

Unfortunately, this commit also breaks an integration test: #3830 (comment)
as `signTransaction` adds too many witnesses — or at least more than
what is needed and more than what we made room for when balancing. Next
commit will fix this by "making more room" by increasing the estimated
`KeyWitnessCount` in a more proper way.
@paweljakubas paweljakubas force-pushed the paweljakubas/adp-2602/shared-wallets-delegation branch from ebc124a to 990aaff Compare April 14, 2023 09:24
paweljakubas pushed a commit that referenced this pull request Apr 17, 2023
Main idea is that we're taking the sum of
1. Key witness count needed for key credentials
2. Key witness count needed for (native) scripts

Unfortunately, this commit also breaks an integration test: #3830 (comment)
as `signTransaction` adds too many witnesses — or at least more than
what is needed and more than what we made room for when balancing. Next
commit will fix this by "making more room" by increasing the estimated
`KeyWitnessCount` in a more proper way.
@paweljakubas paweljakubas force-pushed the paweljakubas/adp-2602/shared-wallets-delegation branch from 990aaff to 2490c5e Compare April 17, 2023 06:46
@@ -453,6 +453,11 @@ instance IsServerError ErrConstructTx where
, "'PATCH /shared-wallets/{walletId}/delegation-script-template'"
, "to make it suitable for constructing transactions."
]
ErrConstructTxStakingInvalid ->
apiError err403 StakingInvalid $ T.unwords
Copy link
Member

Choose a reason for hiding this comment

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

"StakingInvalid", but "lacking a delegation script template" — is there a case for saying "DelegationInvalid" instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines +194 to +195
=> Either XPub (Maybe (Script KeyHash))
-- Reward account public key or optional script hash
Copy link
Member

Choose a reason for hiding this comment

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

Possibly we could use Maybe Cardano.StakeCredential instead? I'll comment more about this in the next PR though.

Copy link
Member

Choose a reason for hiding this comment

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

Right, this contains the actual script as well in contrast to Cardano.StakeCredential.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed, we need script rather than script hash here

@@ -167,10 +169,13 @@ data TransactionLayer k ktype tx = TransactionLayer
, addVkWitnesses
:: AnyCardanoEra
-- Preferred latest era
-> WitnessCountCtx
Copy link
Member

Choose a reason for hiding this comment

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

I hope we can get rid of this with ADP-2675, but is fine for now.

get rid of this

Or we do need the effect of it, but I think it could exist in some more general Map KeyHash XPrv way.

Comment on lines +1876 to +1879
[ expectField (#witnessCount . #scripts)
(`shouldContain` [delegationScript])
, expectField (#witnessCount . #scripts)
(`shouldContain` [paymentScript])
Copy link
Member

Choose a reason for hiding this comment

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

Neat!

Perhaps we could also make sure there aren't any more scripts there through something like this? (haven't tested it)

Suggested change
[ expectField (#witnessCount . #scripts)
(`shouldContain` [delegationScript])
, expectField (#witnessCount . #scripts)
(`shouldContain` [paymentScript])
, expectField (#witnessCount . #scripts . Set.fromList)
(`shouldBe` (Set.fromList [paymentScript, delegationScript]))

verify rSrc
[ expectResponseCode HTTP.status200
, expectField (#direction . #getApiT) (`shouldBe` Outgoing)
, expectField (#status . #getApiT) (`shouldBe` InLedger)
Copy link
Member

Choose a reason for hiding this comment

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

👌

paweljakubas and others added 21 commits April 17, 2023 14:09
more rebase fixes
Main idea is that we're taking the sum of
1. Key witness count needed for key credentials
2. Key witness count needed for (native) scripts

Unfortunately, this commit also breaks an integration test: #3830 (comment)
as `signTransaction` adds too many witnesses — or at least more than
what is needed and more than what we made room for when balancing. Next
commit will fix this by "making more room" by increasing the estimated
`KeyWitnessCount` in a more proper way.
This is a fix for the problem exposed in the previous commit.

For large `Any` script templates the fix in this commit could
significantly increase tx fees, though never more than
≈100 bytes * 44 lovelace/byte * number_of_cosigners. The assumption
made is that without this increase, the txs would have previously
have been invalid anyway though.
@paweljakubas paweljakubas force-pushed the paweljakubas/adp-2602/shared-wallets-delegation branch from 2490c5e to 76cc989 Compare April 17, 2023 12:44
@paweljakubas
Copy link
Contributor Author

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Apr 17, 2023

Build succeeded:

@iohk-bors iohk-bors bot merged commit afc576a into master Apr 17, 2023
@iohk-bors iohk-bors bot deleted the paweljakubas/adp-2602/shared-wallets-delegation branch April 17, 2023 13:53
WilliamKingNoel-Bot pushed a commit that referenced this pull request Apr 17, 2023
…weljakubas <!-- Detail in a few bullet points the work accomplished in this PR. Before you submit, don't forget to: CODE-OF-CONDUCT.md LICENSE README.md bors.toml cabal.project default.nix docker-compose.yml docs flake.lock flake.nix floskell.json hie-direnv.yaml 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 <!--, so that this text does not appear in merge commit messages. CODE-OF-CONDUCT.md LICENSE README.md bors.toml cabal.project default.nix docker-compose.yml docs flake.lock flake.nix floskell.json hie-direnv.yaml lib nix prototypes reports scripts shell.nix specifications test touch.me.CI weeder.dhall Don't waste reviewers' time: ✓ If it's a draft, select the Create Draft PR option. ✓ Self-review your changes to make sure nothing unexpected slipped through. CODE-OF-CONDUCT.md LICENSE README.md bors.toml cabal.project default.nix docker-compose.yml docs flake.lock flake.nix floskell.json hie-direnv.yaml lib nix prototypes reports scripts shell.nix specifications test touch.me.CI weeder.dhall Try to make your intent clear: ✓ Write a good Description that explains what this PR is meant to do. ✓ Jira will detect and link to this PR once created, but you can also link this PR in the description of the corresponding Jira ticket. ✓ Highlight what Testing you have done. ✓ Acknowledge any changes required to the Documentation. --> - [x] sneak delegation action to ctxTx - [x] refactor constraints in constructSharedTx - [x] introduce ErrConstructTxStakingInvalid with integration test - [x] create proper staking credential during tx construction - [x] introduce script as credential in shelley module - [x] introduce txStakingTemplate in TxSkeleton - [x] fee adjustment - [x] inject delegation script if it is valid - [x] add emptySharedWalletDelegating and fixtureSharedWalletdelegating - [x] constructTx with delegation integration test - [x] account deposit/refunds - [x] make sure decodeSharedTransaction account deposits/refunds and certificates properly - [x] extending WitnessCtx in the context of decodeSharedTransaction - [x] signing mechanism - [x] show signing in integration testing - [x] sneaking WitnessCtx in signTransaction - [x] guarding submission and adding proper integration tests ### Comments <!-- Additional comments, links, or screenshots to attach, if any. --> ### Issue Number adp-2602
 <!-- Reference the Jira/GitHub issue that this PR relates to, and which requirements it tackles.
 Note: Jira issues of the form ADP- will be auto-linked. -->
 Co-authored-by: Pawel Jakubas <[email protected]> Co-authored-by: Johannes Lund <[email protected]> Source commit: afc576a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants