-
Notifications
You must be signed in to change notification settings - Fork 115
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
Moving window normalized fee #929
Changes from 2 commits
0742864
9710152
60161d9
c764a38
bd81ab1
1c034cf
1e98ca9
5f6fae8
b8e0bbd
6254133
50b2c0a
03a5c3a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,3 +1,4 @@ | ||||||
import BlockMetadata from '../../models/BlockMetadata'; | ||||||
import IBlockMetadataStore from '../../interfaces/IBlockMetadataStore'; | ||||||
import IFeeCalculator from '../../interfaces/IFeeCalculator'; | ||||||
|
||||||
|
@@ -10,8 +11,8 @@ export default class NormalizedFeeCalculator implements IFeeCalculator { | |||||
private blockMetadataStore: IBlockMetadataStore, | ||||||
private genesisBlockNumber: number, | ||||||
private initialNormalizedFee: number, | ||||||
private lookBackWindowInterval: number, | ||||||
private fluctuationRate: number) {} | ||||||
private feeLookBackWindowInBlocks: number, | ||||||
private feeMaxFluctuationMultiplierPerBlock: number) {} | ||||||
|
||||||
/** | ||||||
* Initializes the Bitcoin processor. | ||||||
|
@@ -21,17 +22,26 @@ export default class NormalizedFeeCalculator implements IFeeCalculator { | |||||
} | ||||||
|
||||||
public async getNormalizedFee (block: number): Promise<number> { | ||||||
// DB call optimization | ||||||
// https://github.com/decentralized-identity/sidetree/issues/936 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: standard patterns is to append
Suggested change
|
||||||
if (block < this.genesisBlockNumber) { | ||||||
// No normalized fee for blocks that exist before genesis | ||||||
return 0; | ||||||
} else if (block < this.genesisBlockNumber + this.lookBackWindowInterval) { | ||||||
} else if (block < this.genesisBlockNumber + this.feeLookBackWindowInBlocks) { | ||||||
// if within look back interval of genesis, use the initial fee | ||||||
return this.initialNormalizedFee; | ||||||
} | ||||||
|
||||||
const blocksToAverage = await this.getLookBackBlocks(block); | ||||||
return this.calculateNormalizedFee(blocksToAverage); | ||||||
} | ||||||
|
||||||
private async getLookBackBlocks (block: number): Promise<BlockMetadata[]> { | ||||||
// look back the interval | ||||||
const blocksToAverage = await this.blockMetadataStore.get(block - this.lookBackWindowInterval, block); | ||||||
return await this.blockMetadataStore.get(block - this.feeLookBackWindowInBlocks, block); | ||||||
} | ||||||
|
||||||
private calculateNormalizedFee (blocksToAverage: BlockMetadata[]): number { | ||||||
let totalFee = 0; | ||||||
let totalTransactionCount = 0; | ||||||
|
||||||
|
@@ -43,22 +53,20 @@ export default class NormalizedFeeCalculator implements IFeeCalculator { | |||||
// TODO: #926 investigate potential rounding differences between languages and implemetations | ||||||
// https://github.com/decentralized-identity/sidetree/issues/926 | ||||||
const unadjustedFee = Math.floor(totalFee / totalTransactionCount); | ||||||
|
||||||
const previousFee = blocksToAverage[blocksToAverage.length - 1].normalizedFee; | ||||||
|
||||||
return this.limitTenPercentPerYear(unadjustedFee, previousFee); | ||||||
return this.adjustFeeToWithinFluctuationRate(unadjustedFee, previousFee); | ||||||
} | ||||||
|
||||||
private limitTenPercentPerYear (unadjustedFee: number, previousFee: number): number { | ||||||
const previousFeeAdjustedUp = Math.floor(previousFee * (1 + this.fluctuationRate)); | ||||||
const previousFeeAdjustedDown = Math.floor(previousFee * (1 - this.fluctuationRate)); | ||||||
private adjustFeeToWithinFluctuationRate (unadjustedFee: number, previousFee: number): number { | ||||||
const maxAllowedFee = Math.floor(previousFee * (1 + this.feeMaxFluctuationMultiplierPerBlock)); | ||||||
const minAllowedFee = Math.floor(previousFee * (1 - this.feeMaxFluctuationMultiplierPerBlock)); | ||||||
|
||||||
if (unadjustedFee > previousFeeAdjustedUp) { | ||||||
return previousFeeAdjustedUp; | ||||||
if (unadjustedFee > maxAllowedFee) { | ||||||
return maxAllowedFee; | ||||||
} | ||||||
|
||||||
if (unadjustedFee < previousFeeAdjustedDown) { | ||||||
return previousFeeAdjustedDown; | ||||||
if (unadjustedFee < minAllowedFee) { | ||||||
return minAllowedFee; | ||||||
} | ||||||
|
||||||
return unadjustedFee; | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -8,8 +8,13 @@ export default interface TransactionModel { | |||||
anchorString: string; | ||||||
transactionFeePaid: number; | ||||||
|
||||||
// Normalized fee sohuld always be populated in core layer when core makes call to transactions endpoint. | ||||||
// It may not be populated in blockchain service. This allows flexibility for the value to be computed on the spot or stored. | ||||||
/** | ||||||
* Normalized fee sohuld always be populated in core layer when core makes call to transactions endpoint. | ||||||
* It may not be populated in blockchain service. This allows flexibility for the value to be computed on the spot or stored. | ||||||
* To remove potentially dangerous behavior. Make a seperate model | ||||||
* https://github.com/decentralized-identity/sidetree/issues/937 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: add TODO for tooling/searching.
Suggested change
|
||||||
*/ | ||||||
|
||||||
normalizedTransactionFee?: number; | ||||||
|
||||||
writer: string; | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,10 +47,12 @@ export default class JasmineSidetreeErrorValidator { | |
expectedContainedStringInMessage?: string | ||
): Promise<void> { | ||
let validated: boolean = false; | ||
let actualError; | ||
|
||
try { | ||
await functionToExecute(); | ||
} catch (e) { | ||
actualError = e; | ||
if (e instanceof SidetreeError) { | ||
expect(e.code).toEqual(expectedErrorCode); | ||
|
||
|
@@ -63,7 +65,7 @@ export default class JasmineSidetreeErrorValidator { | |
} | ||
|
||
if (!validated) { | ||
fail(`Expected error '${expectedErrorCode}' did not occur.`); | ||
fail(`Expected error '${expectedErrorCode}' did not occur. Instead got '${actualError.code}'`); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. LOL! |
||
} | ||
} | ||
} |
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.
I guess
BitcoinProcessor
was the example I was looking for for dependency injection, so I still want to make sure we have an agreed upon pattern. Let's discuss but won't block the PR on this.