-
Notifications
You must be signed in to change notification settings - Fork 137
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
fixfees on grin-wallet #526
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR increases the fee of a tx, will this cause compatibility issues with older wallets? If an older wallet initiates a tx with a newer wallet, will the newer wallet deny it because the fee is too low?
Other than this issue (and the Cargo files) I think the PR looks good.
util/Cargo.toml
Outdated
# grin_util = { path = "../../grin/util"} | ||
# grin_api = { path = "../../grin/api"} | ||
# grin_store = { path = "../../grin/store"} | ||
grin_core = { git = "https://github.com/tromp/grin", branch = "fixfees" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to revert this file once the fixfees PR on grin is merged. Same with the Cargo.lock file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what you point out is not just a wallet issue. nodes running new code will not relay txs paying old fees.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upon further investigation there is no compatibility issue between upgraded and non-upgraded wallets, since the receiving side does not actually check the fee.
👍 from me once we fix the Cargo files after merging mimblewimble/grin#3481.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question about the API change below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 once API conflict above is resolved. Will still require heavy testing in beta.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question about the display of tx_fees below. Other than that I didn't catch anything.
Note: MWC implementation is more simple, because sync with updated node was done before and many issues was addressed at that time. Also, the default base fee is not changed because for MWC we don't change the calculation fee formula.
Complements the fixfees PR at mimblewimble/grin#3481
Still needs some doc tests fixed, which requires some assistance from @yeastplume