Skip to content

feat: add PostageTTL and PostageLabel options to all Beekeeper checks#454

Merged
gacevicljubisa merged 27 commits intomasterfrom
postage-option
Feb 10, 2025
Merged

feat: add PostageTTL and PostageLabel options to all Beekeeper checks#454
gacevicljubisa merged 27 commits intomasterfrom
postage-option

Conversation

@gacevicljubisa
Copy link
Member

No description provided.

@gacevicljubisa gacevicljubisa changed the title feat: enable PostageLabel option for all checks creating batches feat: add PostageTTL and PostageLabel options to all Beekeeper checks Jan 31, 2025
@gacevicljubisa gacevicljubisa marked this pull request as draft January 31, 2025 14:05
@gacevicljubisa gacevicljubisa linked an issue Jan 31, 2025 that may be closed by this pull request
@istae
Copy link
Contributor

istae commented Jan 31, 2025

we should deprecate postage amount altogether

@gacevicljubisa gacevicljubisa marked this pull request as ready for review February 5, 2025 12:01
@gacevicljubisa gacevicljubisa requested review from acha-bill, darkobas2, istae and janos and removed request for acha-bill February 5, 2025 12:01
optionNameConfigGitRepo = "config-git-repo"
optionNameConfigGitUsername = "config-git-username"
optionNameEnableK8S = "enable-k8s"
optionNameGethURL = "geth-url"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename to blockchain-rpc-endpoint

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would require changes in helm charts?

}

timestampPrevious, err := g.fetchBlockTimestamp(ctx, latestBlockNumber-1)
timestampPrevious, err := g.fetchBlockTimestamp(ctx, latestBlockNumber-o.offset)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if latestBlockNumber is less than o.offset ? this could occur in custom environment with new deployments of geth.

Copy link
Contributor

@istae istae Feb 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also by default o.offset is zero, if the option to define it is not used.

Copy link
Member Author

@gacevicljubisa gacevicljubisa Feb 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if latestBlockNumber is less than o.offset ? this could occur in custom environment with new deployments of geth.

Well, I was hoping that RPC will return error in this case. But maybe I can implement some retry logic that can half the offset until it finds the value? What do you think?

Or is it ok to use 0 as the lowest block number?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think using zero should be ok.
you can do something like:

if latestBlockNumber < o.offset {
  g.fetchBlockTimestamp(ctx, 0)
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason, when block 0 is used to query the Geth node, it returns the timestamp 1582023590, which corresponds to February 18, 2020. To avoid this issue, I limited the maximum offset to half of the latest block number.

}

timestampPrevious, err := g.fetchBlockTimestamp(ctx, latestBlockNumber-1)
timestampPrevious, err := g.fetchBlockTimestamp(ctx, latestBlockNumber-o.offset)
Copy link
Contributor

@istae istae Feb 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also by default o.offset is zero, if the option to define it is not used.

Copy link
Contributor

@istae istae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great work, thanks! 🥇

README.md Outdated
--config-git-repo string URL of the Git repository containing configuration files (uses the config-dir if not specified)
--config-git-username string Git username for authentication (required for private repositories)
--enable-k8s Enable Kubernetes client functionality (default true)
--geth-url string URL of the RPC blockchain endpoint
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe URL of the ethereum compatible blockchain RPC endpoint?

log: log,
retry: retryCount,
log: log,
swapClient: opts.SwapClient,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case that opts.SwapClient is nil, use swap.NotSet.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the idea :)

@gacevicljubisa gacevicljubisa requested a review from janos February 7, 2025 16:04
Copy link
Member

@janos janos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! LGTM.

@gacevicljubisa gacevicljubisa merged commit 581ef8b into master Feb 10, 2025
3 checks passed
@gacevicljubisa gacevicljubisa deleted the postage-option branch February 10, 2025 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add PostageTTL Option for All Beekeeper Checks Enable PostageLabel Option for All Beekeeper Checks Creating Batches

4 participants

Comments