Add + implement function to get alias registration fee by blockheight.
Details
Details
- Reviewers
Fabien - Group Reviewers
Restricted Project - Commits
- rABCb48707dcdcc3: [alias-server] Pricing by blockheight
npm test
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| apps/alias-server/constants/alias.js | ||
|---|---|---|
| 13 ↗ | (On Diff #41551) | This is never used |
| apps/alias-server/src/utils.js | ||
| 121 ↗ | (On Diff #41551) | This algo is assuming something about the prices. |
| 126 ↗ | (On Diff #41551) | Nit: the error message is not very clear, it should state that the requested block height is before the alias registration activation height |
| apps/alias-server/test/utilsTests.js | ||
| 175 ↗ | (On Diff #41551) | If you really want to test the function, don't return the same value for multiple inputs... Here you can very well have an off-by-one error and never notice. |
| 224 ↗ | (On Diff #41551) | Please redefine or hardode the value. This is making the test dependent of the above test, so if you remove or change it you will break this one ! |
Comment Actions
Interesting because you fixed an issue that is not the one I was thinking about which is still open.
You algorithm has a strong unchecked assumption, and a better unit test can catch it. I noticed because I would have break the software if I had to update the price myself.
Comment Actions
Confirm startHeight price objects are sorted most recent startHeight to oldest startHeight
Comment Actions
Yes that's the one. In this case I think it's better to return an error rather than sorting at each call, which is costly (and there already is a comment to state this assumption in the constants file). This can be done either once at startup, as close as possible to a static assertion, or during the loop for cheap.
Comment Actions
would be cheapest to do it on startup, also would be nice to just not start the server if a constant is malformed -- however potential edge case if the constants file is changed while the app is running, want to make sure this is caught
| apps/alias-server/constants/alias.js | ||
|---|---|---|
| 22 ↗ | (On Diff #41576) | is the expectation still for Cashtab to check current block heigh is >= startHeight at the same time as retrieving these prices? |
| apps/alias-server/src/utils.js | ||
|---|---|---|
| 120 ↗ | (On Diff #41576) | You can avoid this easily: let lastStartHeight = config.unconfirmedBlockheight
for (let i = 0; i < prices.length; i += 1) {
const { startHeight, fees } = prices[i];
if (startHeight >= lastStartHeight) {
throw new Error('alias price epochs must be sorted by startHeight, highest to lowest');
}
startStartHeight = startHeight;
[...]
} |
Comment Actions
Pushing this change up even though already-green -- implemented the nit approach. In doing this, caught a separate issue in my previous approach -- it's important to iterate through the entire set of price epochs if you are only checking each one against the immediately preceding one. Previous version didn't do this so "worked" on a set of only two price epochs, but would fail on a set with more than 2 (where the first 2 are ordered correctly).
| apps/alias-server/constants/alias.js | ||
|---|---|---|
| 22 ↗ | (On Diff #41576) | Since price changes potentially never happen, seems like a big ask to require implementing wallets to know the tipHeight (also another error vector as these wallets could get it wrong for their own sync issues). I think a good design goal is to have only alias-server dealing with the whole prices object, as alias-server needs to index registration txs at any blockheight. We'll make the full prices object available at an endpoint for any devs who are looking to do something similar. But main approach should be an endpoint like /prices/next that gives the price for the next block (and maybe some sort of warning if there is a price change expected in the next 1000 blocks) This does raise some complications of how to deal with an impending price change. For example, we need to make sure alias-server is returning the right tipHeight and throwing an error if it or its chronik instance is out of sync. However, I think these are manageable ... really only an issue for the transition window of say ~10 blocks before a price change occurs. There is the nuclear option of disabling this endpoint if a price change is coming up and we don't think this is adequately handled. |