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

Fix fee calculation of NativeScript transaction #27

Merged
merged 2 commits into from
Mar 27, 2022

Conversation

siegfried
Copy link
Contributor

No description provided.

@SebastienGllmt
Copy link
Contributor

nit: rename the comment in count_needed_vkeys from "minting keys" -> "native script keys"

nit: you can rename get_mint_scripts to get_native_scripts. It's fine to make breaking changes

@siegfried
Copy link
Contributor Author

Updated. Also, how about adding a set_vkey_number() to TransactionBuilder? For 1/10 multi-sig transaction, the fee is calculated with 10 keys whereas 1 key is enough. Users could use this method to set the count explicitly when it is possible to apply a minimum fee.

@SebastienGllmt
Copy link
Contributor

SebastienGllmt commented Mar 25, 2022

@siegfried yeah I think that's fine to add

we should probably document this stuff in the doc folder. The state of the documentation for this repo is terrible though so I don't blame you if you don't want to try and fix it in this PR

@siegfried
Copy link
Contributor Author

I can do that when I have time, maybe in another PR if you want to merge this.

Copy link
Contributor

@SebastienGllmt SebastienGllmt left a comment

Choose a reason for hiding this comment

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

Merging for now. Other tasks can be done as follow-up

@SebastienGllmt SebastienGllmt merged commit 4d35668 into dcSpark:develop Mar 27, 2022
@siegfried siegfried deleted the set-native-scripts branch March 27, 2022 23:46
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