Skip to content

Traffic-Based App Rewards: Integrate the Reward Computation Trigger with the necessary daml contracts#5119

Open
adetokunbo wants to merge 13 commits intoadetokunbo/cip-104-add-lookup-open-mining-round-by-numberfrom
adetokunbo/cip-104-sv-app-rewards-integrate-reward-computation-trigger
Open

Traffic-Based App Rewards: Integrate the Reward Computation Trigger with the necessary daml contracts#5119
adetokunbo wants to merge 13 commits intoadetokunbo/cip-104-add-lookup-open-mining-round-by-numberfrom
adetokunbo/cip-104-sv-app-rewards-integrate-reward-computation-trigger

Conversation

@adetokunbo
Copy link
Copy Markdown
Contributor

Contributes to #4383
Stacked on #5060

This integrates the RewardComputationTrigger with the Daml contracts from which it obtains the values it uses in its calculations

Pull Request Checklist

Cluster Testing

  • If a cluster test is required, comment /cluster_test on this PR to request it, and ping someone with access to the DA-internal system to approve it.
  • If a hard-migration test is required (from the latest release), comment /hdm_test on this PR to request it, and ping someone with access to the DA-internal system to approve it.
  • If a logical synchronizer upgrade test is required (from canton-3.5), comment /lsu_test on this PR to request it, and ping someone with access to the DA-internal system to approve it.

PR Guidelines

  • Include any change that might be observable by our partners or affect their deployment in the release notes.
  • Specify fixed issues with Fixes #n, and mention issues worked on using #n
  • Include a screenshot for frontend-related PRs - see README or use your favorite screenshot tool

Merge Guidelines

  • Make the git commit message look sensible when squash-merging on GitHub (most likely: just copy your PR description).

@adetokunbo adetokunbo added this to the Traffic-Based App Rewards milestone Apr 20, 2026
@adetokunbo adetokunbo self-assigned this Apr 20, 2026
@adetokunbo adetokunbo force-pushed the adetokunbo/cip-104-add-lookup-open-mining-round-by-number branch from 2da39fa to 9a37516 Compare April 20, 2026 08:59
@adetokunbo adetokunbo force-pushed the adetokunbo/cip-104-sv-app-rewards-integrate-reward-computation-trigger branch from 93367ef to fd731a7 Compare April 20, 2026 09:02
@adetokunbo adetokunbo force-pushed the adetokunbo/cip-104-sv-app-rewards-integrate-reward-computation-trigger branch from f1fc9f1 to eb9c129 Compare April 21, 2026 02:14
@adetokunbo adetokunbo force-pushed the adetokunbo/cip-104-sv-app-rewards-integrate-reward-computation-trigger branch from 66999de to 6a390ef Compare April 23, 2026 06:16
@adetokunbo adetokunbo force-pushed the adetokunbo/cip-104-sv-app-rewards-integrate-reward-computation-trigger branch from 5b32f44 to 68c9cb7 Compare April 23, 2026 21:12
Seq.empty
case Some(contract) =>
RewardComputationInputs.fromOpenMiningRound(contract.payload) match {
case None =>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We have an issue here, the round checked here will actually never get skipped and will block the trigger to process further rounds.
I modified my integ test to add, and then remove rewardConfig from amulet config, and confirmed that trigger gets stuck.
We have to make sure we skip here as the mainnet has been running reward computation, and that will cause latestCompleteO to be Some even in absence of rewardConfig.

Copy link
Copy Markdown
Contributor

@dfordivam dfordivam May 1, 2026

Choose a reason for hiding this comment

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

Actually using earliestRoundWithRewardConfig(Some roundNumber, latestCompleteO) here should work right?
Perhaps we could re-structure the code such that finding the correct round with config is done at both places with single generalized API

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Based on response by Simon yesterday on the doc, we dont need to handle this scenario right now. So lets make sure we truncate the tables on feature branch

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.

2 participants