[WIP] Covenant fees

[rsolari]

Here's a proof-of-concept of my covenant fees proposal. It's not connected to anything, so it doesn't work yet. My goal is to show the scope of the intended changes and get feedback about them.

There are two parts:

• The ComposedEstimator class, which keeps track of four estimators, one for each fee limit. Its job is to pass through arguments to the appropriate PolicyEstimator.
• One interface change in rpc to show the type of changes required changes to fee estimation. This change is just an example. Ultimately, I'd have to change all of the places where something calls estimateFee and estimatePriority.

Does this approach make sense?

[rsolari]

TODOs:

• Add covenantType to 'estimateFee' and 'estimatePriority' more of the calls
• Hook up the new estimator
• Add tests
• Change logger to show which estimator a log line comes from
• Change comparisons to use types instead of typesByVal. Use types throughout
• Fix linting
• Address style nits
• Move PR to other repo

[tynes]

Good work! Looking over this, it looks like you are going in the right direction. Definitely some style nits (we try to keep as close to the hsd style as possible) but you can really have whatever style you want if you aren't planning on opening it as a PR against the other hsd repo. I'd also lean towards trying to use the types for comparisons compared to the typesByVal for comparisons.

[rsolari]

I plan to merge it into the other repo, so I'll follow hsd style guidelines. Are they written down somewhere? I think I just need to change my editor to grab the write config.

[tynes]

eslint is used, there should be an .eslintrc in the root of the repo. Since it is a cryptocurrency project, we do not use the normal eslint in case a random dependency is compromised. We instead use chjj/bslint, which commits to specific versions of exactly what we use to lint.

I cloned https://github.com/chjj/bslint and then installed it globally with npm i -g from within the repo. Then its possible to run npm run lint, as eslint is not a dependency of the project. The paranoid will run the linter in a VM or container.

I will leave comments regarding style, I do not think that the linter will catch all of them. Reading through the codebase a few times will help. Things are generally minimal and there is a focus on performance. For example, short variable names, for ... of over map/filter, Buffers where possible.

[tynes]

Please give feedback on the comments. Ultimately, the final decision to merge code doesn't just come down to me but also Zippy and Boyma. These are based on my interpretation of our style so if you disagree, feel free to have them review as well/read through the codebase more to get your own feel for the style.

[tynes]

I think this should take a Number instead of a string

[tynes]

And we should be very sure that this function doesn't already exist in the codebase someplace

[0xhaven]

You could simplify this by making it a bit more abstract and declaring each "Bucket" and either:

• Storing the bucket in an power-of-two enum and declaring a mapping from each type to its bucket(s)
• Directly declaring the types included in each bucket and having a single method check for membership in each bucket. (i.e. exporting the "Sets" in a lookup-only wrapper class)

[0xhaven]

I actually think this bucketing maybe fits best in the fee estimator, not the rules.

[0xhaven]

It's not clear that none of these buckets will ever intersect. I think it's simplest just to estimate fees in all applicable ways and take their max at the end.

[rsolari]

You mean something like this?

  estimateFee(target, smart, covenantType) {
    const estimates = [];
    if (rules.isOpenLimited(covenantType)) {
      estimates.push(this.openFees.estimateFee(target, smart));
    }
    if (rules.isUpdateLimited(covenantType)) {
      estimates.push(this.updateFees.estimateFee(target, smart));
    }
    if (rules.isRenewalLimited(covenantType)) {
      estimates.push(this.renewalFees.estimateFee(target, smart));
    }

    estimates.push(this.blockspaceFees.estimateFee(target, smart));

    return Math.max(...estimates);
  }

[tynes]

You may find this relevant bcoin-org/bcoin#518

[rsolari]

Thanks for all your feedback @tynes. I made your suggested changes and reopened the PR against the main repo: handshake-org#282