Implementation of anchoring via KAS API #591
Conversation
KimKyungup
requested review from
aidan-kwon,
ehnuje,
jeongkyun-oh,
kjhman21 and
joowon-byun
as code owners
kas/anchor.go
Outdated
| return | ||
| } | ||
|
|
||
| anchor.anchorBlock(block) |
There was a problem hiding this comment.
Doesn't it need to handle error?
There was a problem hiding this comment.
Yes, it is. If there is an error, anchorBlock will log.
In next stage, I will add more feature to retry anchoring in anchorBlcok.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
But for API, the function should return an error.
API will be added in next PR.
kas/anchor.go
Outdated
| return | ||
| } | ||
|
|
||
| anchor.anchorBlock(block) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
kas/anchor.go
Outdated
| return | ||
| } | ||
|
|
||
| anchor.AnchorBlock(block) |
There was a problem hiding this comment.
If the returned error won't be handled, AnchorBlock should not return an error.
There was a problem hiding this comment.
This case doesn't not need to handle. But anchor API will handle the error.
There was a problem hiding this comment.
If the error from AnchorPeriodicBlock does not need to be handled, it is not natural that an error is logged inside AnchorBlock. How about moving out log outside AnchorBlock and leave the log KASAnchor in the API?
There was a problem hiding this comment.
I think they are different between not need to be handled and need to be logged.
Both AnchorPeriodicBlock and API need to log the error.
But for now AnchorPeriodicBlock doesn't need to be handled.
There was a problem hiding this comment.
The point I want to emphasize is that if you don't need to handle the error, that is not an error.
As you don't handle the error from AnchorPeriodicBlock, for me, it should be a warning, not an error.
There was a problem hiding this comment.
OK. I will change the caller chose log or not.
There was a problem hiding this comment.
@ehnuje PTAL. I reflected. :)
@aidan-kwon Thanks. After all implementation PR, I will move this package into the |
|
@jeongkyun-oh @aidan-kwon PTAL :) |
Proposed changes
This PR complete
kaspackage.AnchorPeriodicBlockwill be called by all chain event with a block to anchor periocially.AnchorByBlockNumwill be called by API to anchor specific block.Types of changes
Please put an x in the boxes related to your change.
Checklist
Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.
$ make test)Related issues
Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...