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

Move estimateSignedTxSize to separate module #4037

Merged
merged 2 commits into from
Jul 20, 2023

Conversation

Anviking
Copy link
Member

@Anviking Anviking commented Jul 12, 2023

-- |
-- Copyright: © 2023 IOHK, 2023 Cardano Foundation
-- License: Apache-2.0
--
-- Module for 'signTx' and signing-related utilities for balancing.
module Cardano.Wallet.Write.Tx.Sign
    (
    -- * Signing transactions
    -- TODO: Move signTx function here

    -- * Signing-related utilities required for balancing
      estimateSignedTxSize

    , KeyWitnessCount (..)
    , estimateKeyWitnessCount
    )
    where

Comments

Issue Number

ADP-3081

@Anviking Anviking self-assigned this Jul 12, 2023
@Anviking Anviking changed the base branch from master to anviking/ADP-3081/move-estimateTxSize July 12, 2023 11:51
@Anviking Anviking force-pushed the anviking/ADP-3081/move-estimateSignedTxSize branch from 722509f to 415aecc Compare July 12, 2023 11:53
@Anviking Anviking marked this pull request as ready for review July 12, 2023 11:53
@Anviking Anviking force-pushed the anviking/ADP-3081/move-estimateSignedTxSize branch from 415aecc to d7de4e0 Compare July 12, 2023 13:49
@Anviking Anviking force-pushed the anviking/ADP-3081/move-estimateSignedTxSize branch from d7de4e0 to f8c829a Compare July 13, 2023 11:38
@Anviking Anviking force-pushed the anviking/ADP-3081/move-estimateTxSize branch from 7b24f1f to 16b3ce1 Compare July 13, 2023 11:53
@Anviking Anviking force-pushed the anviking/ADP-3081/move-estimateSignedTxSize branch 2 times, most recently from 84f3987 to b1b9471 Compare July 13, 2023 12:57
estimateSignedTxSize

, KeyWitnessCount (..)
, estimateKeyWitnessCount
Copy link
Member

Choose a reason for hiding this comment

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

Just a thought: the estimateSignedTxSize and estimateKeyWitnessCount functions seem like ideal candidates to place within the Balance.SizeEstimation module, or a more general Balance.Estimation module, given that estimateKeyWitnessCount is not a size estimation, per se.

Copy link
Member Author

@Anviking Anviking Jul 14, 2023

Choose a reason for hiding this comment

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

I did very much want estimateKeyWitnessCount and signTx next to each other. estimateSignedTx might make sense in Write.Tx.SizeEstimation though. I'd suggest to revisit in another PR regardless.

Copy link
Member Author

Choose a reason for hiding this comment

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

estimateSignedTx might make sense in Write.Tx.SizeEstimation though

Actually — no. estimateSignedTx is very different from the functions in the SizeEstimation module. It:

  • is used by balanceTransaction, not coin selection
  • doesn't actually estimate anything itself, it takes the previously estimated KeyWitnessCount as an argument
  • is implemented using evaluateMinimumFee

So it should perhaps be called evaluateSignedTxSize instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did very much want estimateKeyWitnessCount and signTx next to each other.

The reason being that they should match. Or technically:

  • signTx must never add more witnesses than estimateKeyWitnessCount estimated
  • estimateKeyWitnessCount should never overestimate when transactions contain no native scripts

Related: https://cardanofoundation.atlassian.net/browse/ADP-2675

Base automatically changed from anviking/ADP-3081/move-estimateTxSize to master July 14, 2023 11:04
@Anviking Anviking force-pushed the anviking/ADP-3081/move-estimateSignedTxSize branch 2 times, most recently from e9786c2 to 668685b Compare July 17, 2023 12:45
@Anviking Anviking force-pushed the anviking/ADP-3081/move-estimateSignedTxSize branch from 7ebe419 to 46f6c93 Compare July 20, 2023 09:24
@Anviking Anviking enabled auto-merge July 20, 2023 09:24
@Anviking Anviking added this pull request to the merge queue Jul 20, 2023
Merged via the queue into master with commit 40fb32f Jul 20, 2023
2 checks passed
@Anviking Anviking deleted the anviking/ADP-3081/move-estimateSignedTxSize branch July 20, 2023 10:44
github-merge-queue bot pushed a commit that referenced this pull request Jul 20, 2023
### Comments

- Depends on #4037

### Issue Number

ADP-3081
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