Skip to content
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

feat(node): Add node_reward_type field to AddNodePayload and node config #3116

Merged
merged 19 commits into from
Dec 20, 2024

Conversation

sasa-tomic
Copy link
Member

  • Introduce the node_reward_type, to the node configuration and AddNodePayload structures.
  • Update related comments and scripts.

@sasa-tomic sasa-tomic requested review from a team as code owners December 11, 2024 11:52
@github-actions github-actions bot added the feat label Dec 11, 2024
@yvonneanne
Copy link
Contributor

Is this field mandatory?

@sasa-tomic
Copy link
Member Author

Is this field mandatory?

@yvonneanne is this description clear enough to answer the question?
https://github.com/dfinity/ic/pull/3116/files#diff-a09d75efc9b8353b91fad185644d25869fa64baab1b2e546f66e9c0dc0606252R16-R35

@yvonneanne
Copy link
Contributor

Is this field mandatory?

@yvonneanne is this description clear enough to answer the question? https://github.com/dfinity/ic/pull/3116/files#diff-a09d75efc9b8353b91fad185644d25869fa64baab1b2e546f66e9c0dc0606252R16-R35

Thanks, that's definitely helpful information but it doesn't entirely answer my question :)
E.g., does this field need to be set for testnet deployments?

The main reason for my question is Utopia. In many Utopia deployments there will be no rewards for nodes and it is not clear from this PR if there will be direct implications for the Utopia team and some provisioning scripts need to be adjusted. Since I don't remember the details of how we set things up, I'll ask Utopia engineers to have a look at the PR and if necessary start a discussion with you.

@sasa-tomic
Copy link
Member Author

sasa-tomic commented Dec 11, 2024

The main reason for my question is Utopia.

Oh now I understand, thanks for the clarification @yvonneanne . If the reward table is not set in the registry the node_reward_type will not be checked in the registry, so node registration should succeed without the node_reward_type field set. This should cover both testnet and UTOPIA deployments. I've updated the comment in registration.rs to clarify that.
Is this good enough?

@yvonneanne
Copy link
Contributor

The main reason for my question is Utopia.

Oh now I understand, thanks for the clarification @yvonneanne . If the reward table is not set in the registry the node_reward_type will not be checked in the registry, so node registration should succeed without the node_reward_type field set. This should cover both testnet and UTOPIA deployments. I've updated the comment in registration.rs to clarify that. Is this good enough?

perfect, thanks for adding these explanations!

@sasa-tomic sasa-tomic marked this pull request as draft December 18, 2024 10:48
@sasa-tomic sasa-tomic marked this pull request as ready for review December 19, 2024 17:46
@sasa-tomic sasa-tomic enabled auto-merge December 20, 2024 10:06
@sasa-tomic sasa-tomic added this pull request to the merge queue Dec 20, 2024
Merged via the queue into master with commit c0132ba Dec 20, 2024
26 checks passed
@sasa-tomic sasa-tomic deleted the sat-node-reward-type-add-node branch December 20, 2024 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants