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

remove height argument from fee related functions as per fixpastfees RFC #602

Merged
merged 2 commits into from
May 19, 2021

Conversation

tromp
Copy link
Contributor

@tromp tromp commented Apr 5, 2021

name: fixpastfees
about: remove dependence of fee() and fee_shift() functions on height
title: 'fix past fees'

Companion PR for

mimblewimble/grin#3629

both implementing RFC https://github.com/mimblewimble/grin-rfcs/blob/master/text/0021-fix-prior-fees.md
to extend the 40 bit fee restriction and use of fee shifts back beyond HF4 to all history.
Technically a Hard Fork, but if we have a many month reorg introducing a > 40 bit fee prior to HF4, we have bigger problems to deal with than a chain split.

This simplifies all fee related functions as they no longer need a height argument to distinguish preHF4 from postHF4 behaviour.

@tromp tromp changed the title remove height argument from fee related functions as per fixpastfees RFC [DNM] remove height argument from fee related functions as per fixpastfees RFC Apr 5, 2021
@tromp tromp changed the title [DNM] remove height argument from fee related functions as per fixpastfees RFC remove height argument from fee related functions as per fixpastfees RFC May 4, 2021
@phyro
Copy link
Member

phyro commented May 10, 2021

@tromp this one seems to have some failing tests. I also suggest you add the same reviewers as on the grin node fixfees PR

@tromp
Copy link
Contributor Author

tromp commented May 10, 2021

I can't figure out the build failures?!
I click on the error annotations but it just shows the file diff.

@tromp
Copy link
Contributor Author

tromp commented May 11, 2021

how to add reviewers again?

@tromp tromp marked this pull request as draft May 11, 2021 15:47
@tromp tromp marked this pull request as ready for review May 11, 2021 16:37
@GeneFerneau
Copy link
Contributor

GeneFerneau commented May 11, 2021

I can't figure out the build failures?!

Looks like it's pulling in an old version of grin without the fee changes:

error[E0061]: this function takes 1 argument but 0 arguments were supplied
   --> libwallet/src/api_impl/owner.rs:406:17
    |
406 |             Some(f) => f.fee(), // apply fee mask past HF4
    |                          ^^^- supplied 0 arguments
    |                          |
    |                          expected 1 argument
    |
note: associated function defined here
   --> /home/vsts/.cargo/git/checkouts/grin-b002917b6a725898/f6ec77a/core/src/core/transaction.rs:173:9
    |
173 |     pub fn fee(&self, height: u64) -> u64 

Might need to do a test build locally, and commit the Cargo.lock after updating grin dependencies to point to master.

@GeneFerneau
Copy link
Contributor

GeneFerneau commented May 11, 2021

Had to run a cargo update to bump the master version locally, maybe the CI needs it too?

cargo update && \
...

in https://github.com/tromp/grin-wallet/blob/fixpastfees/.ci/general-jobs#L23

similar in:

Do you know of a better way to do this globally for all the test builds?

@tromp
Copy link
Contributor Author

tromp commented May 11, 2021

Oops; i ran a cargo update on my local repo and commited the new Cargo.lock which now conflicts.
I'll await the advice of experts on how to resolve this...

Copy link
Member

@quentinlesceller quentinlesceller left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@quentinlesceller quentinlesceller merged commit b7f587f into mimblewimble:master May 19, 2021
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.

5 participants