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

Remote staking contract bare-bones #37

Merged
merged 2 commits into from
Jun 1, 2023
Merged

Remote staking contract bare-bones #37

merged 2 commits into from
Jun 1, 2023

Conversation

hashedone
Copy link
Collaborator

@hashedone hashedone commented May 31, 2023

Addresses #13

This is just the most basic possible remote staking, with nothing fancy. It meant to address only the first item of #13 (comment).

No tests and no validator choice by design - splitting into separate PRs, smaller = easier to review.

Next step: add the possibility of choosing the validator to stake on passing a message when staking (#39).

@hashedone hashedone marked this pull request as ready for review May 31, 2023 09:27
Copy link
Collaborator

@maurolacy maurolacy left a comment

Choose a reason for hiding this comment

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

LGTM. Just some syntax / naming stuff.

contracts/external-staking/src/msg.rs Outdated Show resolved Hide resolved
contracts/external-staking/src/msg.rs Outdated Show resolved Hide resolved
contracts/external-staking/src/state.rs Outdated Show resolved Hide resolved
contracts/external-staking/src/state.rs Outdated Show resolved Hide resolved
contracts/external-staking/src/state.rs Outdated Show resolved Hide resolved
contracts/external-staking/src/state.rs Outdated Show resolved Hide resolved
contracts/external-staking/src/error.rs Outdated Show resolved Hide resolved
contracts/external-staking/src/cross_staking_api.rs Outdated Show resolved Hide resolved
contracts/external-staking/src/contract.rs Outdated Show resolved Hide resolved
contracts/external-staking/src/contract.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

In general the logic looks good, except for claims.
A number of minor nit-picks, it would be good to clean this up, so we can merge it.
I will look at your other PR, but nice to rebase it on a cleaned/merged version of this

contracts/external-staking/src/cross_staking_api.rs Outdated Show resolved Hide resolved
contracts/external-staking/src/msg.rs Outdated Show resolved Hide resolved
contracts/external-staking/src/state.rs Outdated Show resolved Hide resolved
contracts/external-staking/src/state.rs Outdated Show resolved Hide resolved
contracts/external-staking/src/state.rs Show resolved Hide resolved
contracts/external-staking/src/contract.rs Show resolved Hide resolved
contracts/external-staking/src/contract.rs Outdated Show resolved Hide resolved
contracts/external-staking/src/contract.rs Outdated Show resolved Hide resolved
contracts/external-staking/src/contract.rs Show resolved Hide resolved
@hashedone hashedone merged commit 19d8812 into main Jun 1, 2023
@maurolacy maurolacy deleted the remote-staking branch November 6, 2023 08:27
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.

4 participants