-
Notifications
You must be signed in to change notification settings - Fork 12
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
refactor: revise and add tests for exporter/modules/genesis_deposit.go
#1354
base: staging
Are you sure you want to change the base?
Conversation
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.
Your tests are taking too long to execute
ok github.com/gobitfly/beaconchain/pkg/exporter/modules 71.193s coverage: 0.7% of statements
It should not be over, let say, 5s (and it's very generous)
client: client, | ||
db: db, | ||
offset: time.Minute, | ||
ctx: context.Background(), |
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.
ctx should be a param of Export
The usage would be
// in main.go
ctx, cancel := context.WithCancel(context.Background())
go modules.StartAll(ctx, ...)
// wait for something
cancel()
// in backend/pkg/exporter/modules/base.go
func StartAll(ctx context.Context, ...) {
...
go newGenesisDepositsExporter(context.ConsClient, dbs).Export(ctx)
This a good change but I think it's too soon.
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.
In the end this is probably not needed, we have an exit condition on this go routine with
if genesisDepositCount > 0 {
return
}
backend/pkg/commons/db/db.go
Outdated
type ConsensusDBI interface { | ||
GetLatestEpoch() (uint64, error) | ||
GetDepositsCountForBlockSlot() (uint64, error) | ||
SaveBlockDeposits(validatorIndex uint64, pubkey []byte, withdrawalCredentials []byte, balance uint64) error |
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.
this should accept types.StandardValidator
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.
using types.StandardValidator
triggers a lint error due to a dependency rule tho
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.
Even with this syntax ?
import (
constypes "github.com/gobitfly/beaconchain/pkg/consapi/types"
)
func (d *ConsensusDB) SaveBlockDeposits(validator constypes.StandardValidator) error {
...
}
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.
Yeah
No description provided.