Feature/extends capacity refactor #395
Conversation
luissanchez0305
requested review from
AuHau and
vojtechsimetka
as code owners
There was a problem hiding this comment.
It is a step in the right direction, but there are still some things to fix to get the clean structure that we want.
Generally, please look at the visibility on the class's properties and methods. Most of those can be private and following the encapsulation principle, I would argue that they should be private. I have commented on few of the places where I saw that could be private, but generally please go through the classes and verify what needs to be public/private.
The class hierarchy is now much nicer, but as mentioned in one of the comment, please introduce StampManager interface and do some shuffling on the base class.
Another thing is the config input validation and parsing. Please! Do it properly! See the comments.
Lastly, again, testing. I have told you what and how you should test, so please do it. We can discuss it on call if you need to remind yourself.
|
I have done some black box testing, my notes in #361 have been addressed. Could not trigger over purchase, or overlapping dilutes and topups 👍 There are some less useful edge cases though. In autobuy mode, gateway-proxy makes sure I receive a new stamp before my previous runs out. In autoextend mode, this is a bit different. Having autoextend with dilute & without topup, and a stamp at 1 second TTL and 99% usage, gateway-proxy will choose to dilute that stamp. This is an unnecessary operation, the stamp will expire anyway, the correct one would have been to buy the new one already. Having autoextend with topup & without dilute, and a stamp at 1 second TTL and 99% usage, gateway-proxy will correctly choose to top up that stamp to keep the data living. 👍 However, I cannot upload further content as no new stamp will be bought. IMO this should not be addressed in code immediately, but is a discussion topic we should touch upon. If we allow autoextend mode to run with only one operation, we should have good UX for them. In the current state, I'd document that we suggest either autobuy mode, or autoextend but with both dilute and topup. |
|
Also please take a look at the crash when |
| export class BaseStampManager { | ||
| public stamp?: string | ||
| public interval?: ReturnType<typeof setInterval> | ||
| public usableStamps?: PostageBatch[] |
| if (this.stamp) { | ||
| const stamp = this.stamp | ||
| logger.info('using hardcoded stamp', { stamp }) | ||
|
|
||
| return stamp | ||
| } |
There was a problem hiding this comment.
This should be part of the HardcodedStampManager.
src/stamps/extends.ts
Outdated
| return [] | ||
| } | ||
|
|
||
| export async function topUpStamp(beeDebug: BeeDebug, postageBatchId: string, amount: string): Promise<PostageBatch> { |
3 participants
Closes #311
Basically, instead of buying a new postage stamp every time the usage threshold is reached with this feature, it would be possible to extend the capacity (depth) of the postage stamp using the dilute endpoint.
Also, refactor the code base to understand better the stamps features