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.

Sign up for GitHub

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

Jump to bottom

Expose min fee from node #2920

Merged
merged 11 commits into from
Sep 30, 2021
Merged

Expose min fee from node #2920

merged 11 commits into from
Sep 30, 2021

Conversation

paweljakubas
Copy link
Contributor

@paweljakubas paweljakubas commented Sep 23, 2021 •
edited
Loading

adp-1143

evaluateTransactionFee from cardano-api uses cardano-api's ProtocolParamters. Hence,

  1. cardano-api's ProtocolParameters were exposed via new network method DONE
  2. evaluateMinimumFee was added to transaction interface DONE
  3. impl in shelley using cardano-api function was used using cardano-api's ProtocolParameters and SealedTx DONE
  4. function determining the number of witnesses was implemented DONE
  5. try to address calculation of number of certificate wits (using ledger things) DONE

Limitations:

  • not for ByronEra and not for Byron witnesses

Comments

Issue Number

@paweljakubas paweljakubas self-assigned this Sep 23, 2021
@Anviking Anviking self-requested a review September 24, 2021 17:02
@paweljakubas paweljakubas force-pushed the paweljakubas/adp-1143/expose-min-fee-from-node branch from 78bb514 to 7a79ee7 Compare September 28, 2021 12:34
@paweljakubas paweljakubas marked this pull request as ready for review September 28, 2021 12:38
@paweljakubas paweljakubas force-pushed the paweljakubas/adp-1143/expose-min-fee-from-node branch from 73b30ca to cfc1133 Compare September 29, 2021 13:21
Anviking
Anviking reviewed Sep 29, 2021
View reviewed changes
lib/shelley/src/Cardano/Wallet/Shelley/Network.hs Outdated
return (pp, sp)

ppNode <- onAnyEra
(error "not sure at this moment how to handle that")
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 make this Nothing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can. good idea

@paweljakubas paweljakubas force-pushed the paweljakubas/adp-1143/expose-min-fee-from-node branch from cfc1133 to 585c331 Compare September 29, 2021 18:38
@paweljakubas paweljakubas force-pushed the paweljakubas/adp-1143/expose-min-fee-from-node branch from ab20b86 to 69d3f98 Compare September 30, 2021 07:27
@paweljakubas paweljakubas force-pushed the paweljakubas/adp-1143/expose-min-fee-from-node branch from 69d3f98 to ec8e1f0 Compare September 30, 2021 09:12
Anviking
Anviking reviewed Sep 30, 2021
View reviewed changes
lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs
fromCardanoLovelace minFee
where
minFee = case getSealedTxBody tx of
InAnyCardanoEra ShelleyEra txbody ->
Copy link
Member

Choose a reason for hiding this comment

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

I'm looking into this, but I don't think we need to expose the ledger txbodies here... The Cardano.Tx body wits pattern should be enough...

Copy link
Member

Choose a reason for hiding this comment

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

Or is it intentional for some reason?

Copy link
Member

Choose a reason for hiding this comment

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

The required IsShelleyBasedEra era, I think we should be able to conjure up somehow...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unfortunately,in the case of tx certs I cannot extract from cardano-node due to how it sneaks BuildTx there. And it turn out that doing this through ledger tx is a way...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

feel free to try without ledger bypass if you have a while

Copy link
Member

Choose a reason for hiding this comment

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

For the record, what I had missed until our quick call was:

Anviking
Anviking reviewed Sep 30, 2021
View reviewed changes
lib/core/src/Cardano/Wallet/Transaction.hs Outdated
-- Current protocol parameters
-> tx
-- The sealed transaction
-> Coin
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should return Maybe instead of erroring in case of byron, but I think leaving it as-is is fine while still trying to fit the pieces together.

Copy link
Member

Choose a reason for hiding this comment

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

I ended up adding a Maybe here.

Anviking
Anviking reviewed Sep 30, 2021
View reviewed changes
lib/shelley/src/Cardano/Wallet/Shelley/Compatibility.hs
@@ -716,6 +722,166 @@ txParametersFromPParams maxBundleSize pp = W.TxParameters
naturalToDouble :: Natural -> Double
naturalToDouble = fromIntegral

--------------------------------------------------------------------------------
-- Copied from cardano-api
-- To be removed when once again exposed.
Copy link
Member

Choose a reason for hiding this comment

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

I added this note for future readers

Anviking
Anviking reviewed Sep 30, 2021
View reviewed changes
lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs
in Cardano.evaluateTransactionFee @Cardano.MaryEra pp txbody (witsNum txbody certNum) 0
InAnyCardanoEra AlonzoEra txbody ->
let (Cardano.ShelleyTxBody _ ledgertxbody _ _ _ _) = txbody
certNum = length $ Alonzo.txcerts ledgertxbody
Copy link
Member

Choose a reason for hiding this comment

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

I think these certNum could be a slight overestimate, as not all certs require witnesses (KeyReg being the exception). This should affect the result of _evaluateMinimumFee when Daedalus users delegate for the first time.

Another, more theoretical case would also be where all the certs require just a single key witness.

Not a big deal, but could add a comment about it.

Copy link
Member

Choose a reason for hiding this comment

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

6faea90

Anviking
Anviking approved these changes Sep 30, 2021
View reviewed changes
Copy link
Member

@Anviking Anviking left a comment

Choose a reason for hiding this comment

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

LGTM, will be useful to have currentNodeProtocolParameters

@Anviking
Add comments and Maybe
9876e47 
1. Make evaluateMinimumFee return `Maybe Coin` rather than being a partial
function.

2. Add some comments clarifying aspects.
@Anviking Anviking force-pushed the paweljakubas/adp-1143/expose-min-fee-from-node branch from 6faea90 to 9876e47 Compare September 30, 2021 13:08
@Anviking
Copy link
Member

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Sep 30, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit e2f3d5b into master Sep 30, 2021
@iohk-bors iohk-bors bot deleted the paweljakubas/adp-1143/expose-min-fee-from-node branch September 30, 2021 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Anviking Anviking Anviking approved these changes

Assignees

@paweljakubas paweljakubas

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@paweljakubas @Anviking