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

Handle key deposit for Shelley transactions #1806

Merged
merged 6 commits into from
Jun 26, 2020

Conversation

KtorZ
Copy link
Member

@KtorZ KtorZ commented Jun 26, 2020

Issue Number

#1799

Overview

  • 8085f82
    📍 add a non-null key deposit to genesis, and account for it for launching pools

  • 8240a15
    📍 account for key deposit when registering a stake key
    This is.. tricky, because the deposit is sort of implicit but, because of the dual support with Jörmungandr, we have to treat it as a fee. So, there's a bit of weird logic / work-around when creating the actual transaction.

  • 285d027
    📍 fix fee calculation for Shelley

    • using 28-byte hash length for key hashes
  • adding certificate witnesses to the estimation

  • 0495b96
    📍 give deposit back when de-registing a stake key

  • a19bc0e
    📍 adjust test scenarios according to recent API changes

Comments

Not 100% ideal, but works without too much added complexity.

This is.. tricky, because the deposit is sort of implicit but, because of the dual support with Jörmungandr, we have to treat it as a fee. So, there's a bit of weird logic / work-around when creating the actual transaction.
- using 28-byte hash length for key hashes
- adding certificate witnesses to the estimation
@KtorZ KtorZ requested a review from paweljakubas June 26, 2020 17:33
@KtorZ KtorZ self-assigned this Jun 26, 2020
@KtorZ KtorZ force-pushed the KtorZ/1799/key-deposit-shelley-transactions branch from a19bc0e to 3564517 Compare June 26, 2020 17:34
@@ -31,7 +31,7 @@ protocolParams:
extraEntropy:
tag: NeutralNonce
maxBlockHeaderSize: 217569
keyDeposit: 0
keyDeposit: 100
Copy link
Member Author

Choose a reason for hiding this comment

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

This little one made pretty much all integration tests to fail, as they should.

, if null certs
then Set.empty
else Set.singleton (dummyWitness dummyTxIn)
)
Copy link
Member Author

Choose a reason for hiding this comment

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

Had to revise the size estimation too because it was wrong :/ ...

a) The hash length were still 32 bytes
b) Certificate witnesses weren't counted

Turned out that, until I introduced the deposit amount, b) and a) were both balancing each other (we were allocating too much fee because of the wrong hash sizes, and it was coping with the lack of witness for the certificate 🤷 )

@KtorZ KtorZ force-pushed the KtorZ/1799/key-deposit-shelley-transactions branch from 3564517 to 0784356 Compare June 26, 2020 17:59
@KtorZ
Copy link
Member Author

KtorZ commented Jun 26, 2020

bors try

iohk-bors bot added a commit that referenced this pull request Jun 26, 2020
@KtorZ KtorZ added the RESOLVING ISSUE Mark a PR as resolving issues, for auto-generated CHANGELOG label Jun 26, 2020
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jun 26, 2020

try

Build failed

@KtorZ
Copy link
Member Author

KtorZ commented Jun 26, 2020

bors try

iohk-bors bot added a commit that referenced this pull request Jun 26, 2020
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jun 26, 2020

try

Build failed

@KtorZ
Copy link
Member Author

KtorZ commented Jun 26, 2020

bors retry

iohk-bors bot added a commit that referenced this pull request Jun 26, 2020
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jun 26, 2020

@KtorZ KtorZ merged commit 6cb6f06 into master Jun 26, 2020
@KtorZ KtorZ deleted the KtorZ/1799/key-deposit-shelley-transactions branch June 26, 2020 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RESOLVING ISSUE Mark a PR as resolving issues, for auto-generated CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant