Construct/sign/submit tx with delegation shared wallets #3738
Conversation
paweljakubas
force-pushed
the
paweljakubas/adp-2602/construct-tx-with-delegation-shared-wallets
branch
2 times, most recently
from
51fef79
to
de18bc7
Compare
paweljakubas
changed the title
paweljakubas
force-pushed
the
paweljakubas/adp-2602/construct-tx-with-delegation-shared-wallets
branch
2 times, most recently
from
050ad1b
to
c8fb348
Compare
paweljakubas
force-pushed
the
paweljakubas/adp-2602/construct-tx-with-delegation-shared-wallets
branch
from
dd0e09d
to
5e290cd
Compare
| toCANetworkDistriminant | ||
| :: forall (n :: NetworkDiscriminant). Typeable n | ||
| => CA.NetworkDiscriminant CA.Shelley | ||
| toCANetworkDistriminant = |
There was a problem hiding this comment.
This function seems unused. Do you have plans for it, or should it be removed?
| <$ filter ourRewardAccountRegistration certs | ||
| , depositsReturned = | ||
| (Quantity . fromIntegral . unCoin . W.stakeKeyDeposit $ pp) | ||
| <$ filter ourRewardAccountDeregistration certs |
There was a problem hiding this comment.
This does not expose external deposits and returns. Should it not? Otherwise the tx may look unbalanced based on the response.
| decodeTx tl era witCountCtx sealedTx | ||
|
|
||
| let numberStakingNativeScripts = | ||
| length $ filter hasDelegationKeyHash nativeScripts |
There was a problem hiding this comment.
Why not get this from delegationScriptTemplateM?
| @@ -2141,6 +2229,7 @@ estimateTxSize era skeleton = | |||
| + sizeOf_VKeyWitnesses | |||
| + sizeOf_NativeScripts txMintOrBurnScripts | |||
| + maybe 0 (determinePaymentTemplateSize txMintOrBurnScripts) txPaymentTemplate | |||
| + maybe 0 determineStakingTemplateSize txStakingTemplate | |||
There was a problem hiding this comment.
This is not seen by balanceTx, so any change to estimateTxSize should not be needed.
| let accXPub = getRawKey $ Shared.accountXPub s | ||
| let xpub = CA.getKey $ | ||
| deriveDelegationPublicKey (CA.liftXPub accXPub) minBound | ||
| let delCred = maybeToRight xpub scriptM |
There was a problem hiding this comment.
With ErrConstructTxStakingInvalid, I don't think we can ever hit the xpub case? If so, I suspect an error would be clearer (and obviously, rewriting everything such that the error is not needed would be even clearer, but I think that's for the future.)
| let | ||
| rewardAcnt :: (XPrv, Passphrase "encryption") | ||
| rewardAcnt = | ||
| (getRawKey $ deriveRewardAccount @k rootPwd rootKey, rootPwd) | ||
| (getRawKey $ deriveRewardAccount @k rootPwd rootKey minBound, rootPwd) |
There was a problem hiding this comment.
I believe rewardAcnt is redundant in the case of shared wallets; if we stake the XPrv for signing withdrawals and certificates will always be the same. This doesn't really matter, but could be confusing. I suspect it could be fixed now, but otherwise ADP-2675 should make this easier to deal with.
| Cardano.StakeAddressDeregistrationCertificate cred -> | ||
| estimateWitNumForCred cred | ||
| Cardano.StakeAddressDelegationCertificate cred _ -> | ||
| estimateWitNumForCred cred |
There was a problem hiding this comment.
I don't think we need to resolve the scripts from ByScript credentials. We already have scriptVkWitsUpperBound, and could handle it in a simpler and possible safer way by:
- Make
estimateWitNumForCredonly return1for key credentials and 0 otherwise. - Drop
apiScriptHashes- we don't need the resolution
I tried this locally and it fails with (FeeTooSmallUTxO (Coin 176900) (Coin 166800). But I suspect this is because we somehow sign with one witness too much, probably because of duplication between:
, mapMaybe mkWdrlCertWitness certs
, mapMaybe mkStakingScriptWitness stakingScriptsKeyHashes
|
Haven't made the final review (waiting for response now & happy to chat), but I wanted to also point out / emphasise overall that I really feel
would make the logic in this PR much neater and is something we should do fairly soon. |
| decodeErrorInfo rTx `shouldBe` StakingInvalid | ||
|
|
||
| it "SHARED_TRANSACTIONS_DELEGATION_01a - \ | ||
| \Can join stakepool, rejoin another and quit" $ \ctx -> runResourceT $ do |
There was a problem hiding this comment.
Integration tests only covers join atm. rejoin and quit are missing if I'm not mistaken.
Also, I've noticed (after joining shared wallet on preprod) that the delegation settings returned as part of getSharedWallet are not updated and still show:
"delegation": {
"active": {
"status": "not_delegating"
},
"next": []
}
Therefore we need to make sure to also check something like this.
|
closing in favor of #3830 |
paweljakubas
deleted the
paweljakubas/adp-2602/construct-tx-with-delegation-shared-wallets
branch
Comments
Issue Number
adp-2602