Add integrate `spark-rewards` #338

juliangruber · 2024-08-30T11:06:48Z

CheckerNetwork/roadmap#161
Closes #364

bajtos

See the comment below.

GitHub also shows a bunch of TypeScript errors. Did you run the npm test?

bin/dry-run.js

juliangruber · 2024-08-30T14:17:04Z

See the comment below.

GitHub also shows a bunch of TypeScript errors. Did you run the npm test?

This is a draft and I hadn't requested a review yet :) Also see the commit title

bajtos

Great work!

I have a few comments to discuss, PTAL below.

bajtos · 2024-10-03T12:58:28Z

bin/spark-evaluate.js

-  // Bypass NonceManager as we need to cancel transactions with the same nonce
-  signer: wallet
-})
+const submitScores = async (participants, values) => {


Can you please move this function to lib? I'd like to keep bin files as high-level glue connecting bits imported from lib.

Is it worth adding a test for it? (E.g. using a mocked fetch.)

I've refactored the function, I think adding tests is lower priority due to the simplicity of the function, therefore I added a comment about adding tests in the future instead

I've refactored the function,

Thank you!

I think adding tests is lower priority due to the simplicity of the function, therefore I added a comment about adding tests in the future instead

I am curious - have you ever run this code, or will we execute it for the first time in production?

We did the latter recently (#363, landed on Sept 24th) and there was a bug we did not discover & fix until the next day (#368).

Please watch the production after landing this PR until the first round is evaluated & scores are submitted to ensure the code works as intended.

bajtos · 2024-10-03T13:05:27Z

test/evaluate.js

+    const submitScoresCalls = []
+    const submitScores = async (participantAddresses, scores) => {
+      submitScoresCalls.push({ participantAddresses, scores })
+    }


Can you preserve the name setScoresCall to reduce the code churn?

I am surprised we don't need to include the roundIndex when submitting scores. Could the lack of roundIndex make debugging more difficult for us?

How do you feel about keeping the setScores signature in the interfaces, i.e. modify submitScores to require (roundIndex, participantAddresses, scores), even if the actual implementation calling spark-rewards does not use the round index?

Since it's something used by the previous version, I think we may need to have it again in the future. I know it's just a speculation violating YAGNI principle, so 🤷🏻 What I like most is that if we preserve the signature then there will be much fewer changes in the existing tests.

Can you preserve the name setScoresCall to reduce the code churn?

This would mean we have a function submitScores that pushes into setScoresCalls. That doesn't make sense. It's only coherent if we also rename submitScores to setScores everywhere. I'm ok with that, wdyt?

I am surprised we don't need to include the roundIndex when submitting scores. Could the lack of roundIndex make debugging more difficult for us?

spark-rewards has no notion of rounds. What would we need the round index for?

How do you feel about keeping the setScores signature in the interfaces, i.e. modify submitScores to require (roundIndex, participantAddresses, scores), even if the actual implementation calling spark-rewards does not use the round index?

We don't need that right now so I'm against it and think we shouldn't spend time discussing it. If you feel strongly I'll just change it.

Can you preserve the name setScoresCall to reduce the code churn?

This would mean we have a function submitScores that pushes into setScoresCalls. That doesn't make sense. It's only coherent if we also rename submitScores to setScores everywhere. I'm ok with that, wdyt?

rename submitScores to setScores everywhere sounds good to me 👍🏻

I am surprised we don't need to include the roundIndex when submitting scores. Could the lack of roundIndex make debugging more difficult for us?

spark-rewards has no notion of rounds. What would we need the round index for?

For troubleshooting - e.g. to detect a duplicate submission of scores for the same round, or a round with no scores submitted.

It also allows us to implement idempotency in spark-rewards: we can add pRetry to spark-evaluate and drop duplicate scores for the same round on the spark-rewards side.

How do you feel about keeping the setScores signature in the interfaces, i.e. modify submitScores to require (roundIndex, participantAddresses, scores), even if the actual implementation calling spark-rewards does not use the round index?

We don't need that right now so I'm against it and think we shouldn't spend time discussing it. If you feel strongly I'll just change it.

Ack!

rename submitScores to setScores everywhere sounds good to me 👍🏻

Done!

For troubleshooting - e.g. to detect a duplicate submission of scores for the same round, or a round with no scores submitted.

It also allows us to implement idempotency in spark-rewards: we can add pRetry to spark-evaluate and drop duplicate scores for the same round on the spark-rewards side.

I opened a new issue for this, I don't see it as release blocking: CheckerNetwork/spark-rewards#15

bajtos · 2024-10-04T07:50:38Z

@juliangruber could you please remind me what's the plan for paying out rewards tracked by spark-rewards and how are we going to handle the migration - what are the next steps after landing this PR up to the first payout of rewards?

juliangruber · 2024-10-04T11:01:27Z

@juliangruber could you please remind me what's the plan for paying out rewards tracked by spark-rewards

Docs for reference: https://spacemeridian.notion.site/Meridian-Off-chain-scheduled-rewards-f9480ef009464ecfaf02a4f752cc4d70#6b1b57295e3046a395085473027d8942

The steps are:

GET /scores on spark-rewards
addBalances() on the IE
POST /paid on spark-rewards

We should write a CLI for this, as doing this manually is brittle. I'll do that next.

and how are we going to handle the migration - what are the next steps after landing this PR up to the first payout of rewards?

We don't need to migrate anything, as calling releaseRewards() on the IE will release rewards both from before and after merging this PR. I will take care of updating the docs.

bajtos · 2024-10-07T10:25:49Z

@juliangruber could you please remind me what's the plan for paying out rewards tracked by spark-rewards

Docs for reference: https://spacemeridian.notion.site/Meridian-Off-chain-scheduled-rewards-f9480ef009464ecfaf02a4f752cc4d70#6b1b57295e3046a395085473027d8942

The steps are:

GET /scores on spark-rewards

addBalances() on the IE

POST /paid on spark-rewards

We should write a CLI for this, as doing this manually is brittle. I'll do that next.

and how are we going to handle the migration - what are the next steps after landing this PR up to the first payout of rewards?

We don't need to migrate anything, as calling releaseRewards() on the IE will release rewards both from before and after merging this PR. I will take care of updating the docs.

Perfect!

Do we need to make any changes inside Station Core and/or Station Desktop to let the chart in the Station Desktop UI to show the correct amount of scheduled rewards? This amount can be a combination of scheduled rewards tracked by the smart contract and by spark-rewards.

bajtos

The new version is good enough 👍🏻

Please take a look at #338 (comment) before landing.

EDIT

Also please consider the impact on Station Desktop UI before landing, see #338 (comment).

juliangruber · 2024-10-07T13:38:54Z

Converted to draft as these projects should have spark-rewards integrated first:

core
spark-stats
telegraf

add integrate spark-rewards (wip)

78449c8

bajtos requested changes Aug 30, 2024

View reviewed changes

bin/dry-run.js Outdated Show resolved Hide resolved

juliangruber added 6 commits October 2, 2024 13:00

Merge branch 'main' into add/spark-rewards

6994ef0

clean up

f79d1ee

add submitScores()

a349a27

fix type tests

901d42c

fix lint, clean up

9ad1796

fix tests

5ae4356

juliangruber force-pushed the add/spark-rewards branch from 8c897a6 to 5ae4356 Compare October 2, 2024 11:58

juliangruber requested a review from bajtos October 2, 2024 11:59

juliangruber marked this pull request as ready for review October 2, 2024 11:59

style

b6e3c73

bajtos requested changes Oct 3, 2024

View reviewed changes

juliangruber added 2 commits October 4, 2024 12:54

refactor submitScores

ecd910a

todo

3157d37

juliangruber requested a review from bajtos October 4, 2024 11:01

bajtos approved these changes Oct 7, 2024

View reviewed changes

juliangruber marked this pull request as draft October 7, 2024 13:38

This comment was marked as outdated.

Sign in to view

juliangruber added the blocked label Oct 11, 2024

juliangruber mentioned this pull request Oct 11, 2024

Move L1 scores to spark-rewards CheckerNetwork/roadmap#161

Closed

4 tasks

juliangruber marked this pull request as ready for review October 13, 2024 08:59

juliangruber added 2 commits October 13, 2024 10:59

Merge branch 'main' into add/spark-rewards

850fe12

submitScores() -> setScores()

b5a7a13

juliangruber merged commit 5da35f5 into main Oct 13, 2024
6 checks passed

juliangruber deleted the add/spark-rewards branch October 13, 2024 09:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add integrate `spark-rewards` #338

Add integrate `spark-rewards` #338

juliangruber commented Aug 30, 2024 •

edited

Loading

bajtos left a comment

juliangruber commented Aug 30, 2024

bajtos left a comment

bajtos Oct 3, 2024

juliangruber Oct 4, 2024

bajtos Oct 7, 2024

bajtos Oct 3, 2024

juliangruber Oct 4, 2024

bajtos Oct 7, 2024

juliangruber Oct 13, 2024

bajtos commented Oct 4, 2024 •

edited

Loading

juliangruber commented Oct 4, 2024

bajtos commented Oct 7, 2024

bajtos left a comment •

edited

Loading

juliangruber commented Oct 7, 2024 •

edited

Loading

This comment was marked as outdated.

Add integrate spark-rewards #338

Add integrate spark-rewards #338

Conversation

juliangruber commented Aug 30, 2024 • edited Loading

bajtos left a comment

Choose a reason for hiding this comment

juliangruber commented Aug 30, 2024

bajtos left a comment

Choose a reason for hiding this comment

bajtos Oct 3, 2024

Choose a reason for hiding this comment

juliangruber Oct 4, 2024

Choose a reason for hiding this comment

bajtos Oct 7, 2024

Choose a reason for hiding this comment

bajtos Oct 3, 2024

Choose a reason for hiding this comment

juliangruber Oct 4, 2024

Choose a reason for hiding this comment

bajtos Oct 7, 2024

Choose a reason for hiding this comment

juliangruber Oct 13, 2024

Choose a reason for hiding this comment

bajtos commented Oct 4, 2024 • edited Loading

juliangruber commented Oct 4, 2024

bajtos commented Oct 7, 2024

bajtos left a comment • edited Loading

Choose a reason for hiding this comment

juliangruber commented Oct 7, 2024 • edited Loading

This comment was marked as outdated.

Add integrate `spark-rewards` #338

Add integrate `spark-rewards` #338

juliangruber commented Aug 30, 2024 •

edited

Loading

bajtos commented Oct 4, 2024 •

edited

Loading

bajtos left a comment •

edited

Loading

juliangruber commented Oct 7, 2024 •

edited

Loading