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.

Sign up for GitHub

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

Jump to bottom

F/value ranges external staking #108

Merged
merged 7 commits into from
Aug 23, 2023
Merged

F/value ranges external staking #108

merged 7 commits into from
Aug 23, 2023

Conversation

jawoznia
Copy link
Contributor

No description provided.

@jawoznia jawoznia force-pushed the f/value-ranges-external-staking branch from 0f0ff02 to cc5d4d3 Compare August 21, 2023 11:16
maurolacy
maurolacy reviewed Aug 21, 2023
View reviewed changes
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.

Didn't review all. Publishing because there's an error in the cross staking module.

Cargo.toml Outdated Show resolved Hide resolved
contracts/provider/external-staking/src/msg.rs Show resolved Hide resolved
contracts/provider/external-staking/src/lib.rs Outdated Show resolved Hide resolved
contracts/provider/external-staking/src/contract.rs Show resolved Hide resolved
contracts/provider/external-staking/src/contract.rs Show resolved Hide resolved
contracts/provider/external-staking/src/cross_staking.rs Outdated Show resolved Hide resolved
contracts/provider/external-staking/src/cross_staking.rs Outdated Show resolved Hide resolved
@jawoznia jawoznia force-pushed the f/value-ranges-external-staking branch 3 times, most recently from e646004 to 69c4ac2 Compare August 21, 2023 16:37
maurolacy
maurolacy reviewed Aug 21, 2023
View reviewed changes
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.

I think your code isn't ready yet as it's still in draft, but did a 2nd review pass just out of curiosity.

contracts/provider/vault/src/contract.rs Outdated Show resolved Hide resolved
contracts/provider/external-staking/src/contract.rs Outdated Show resolved Hide resolved
contracts/provider/external-staking/src/contract.rs Outdated Show resolved Hide resolved
contracts/provider/external-staking/src/contract.rs Outdated Show resolved Hide resolved
contracts/provider/external-staking/src/contract.rs Outdated Show resolved Hide resolved
contracts/provider/external-staking/src/contract.rs Outdated Show resolved Hide resolved
contracts/provider/external-staking/src/contract.rs Outdated Show resolved Hide resolved
contracts/provider/external-staking/src/contract.rs Show resolved Hide resolved
contracts/provider/external-staking/src/contract.rs Outdated Show resolved Hide resolved
maurolacy
maurolacy reviewed Aug 22, 2023
View reviewed changes
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.

Looks good.

Will take a look at tests later.

contracts/provider/external-staking/src/contract.rs Outdated Show resolved Hide resolved
contracts/provider/external-staking/src/contract.rs Show resolved Hide resolved
contracts/provider/external-staking/src/contract.rs Outdated Show resolved Hide resolved
contracts/provider/external-staking/src/contract.rs Show resolved Hide resolved
maurolacy
maurolacy approved these changes Aug 22, 2023
View reviewed changes
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.

Reviewed tests. Looks good.

There's still a Lock error that can be removed. And a MaybePendingRewards enum that can be turned into a PendingRerwards struct. I can do this last one if you want.

contracts/provider/external-staking/src/error.rs Outdated Show resolved Hide resolved
contracts/provider/external-staking/src/multitest.rs Outdated Show resolved Hide resolved
contracts/provider/external-staking/src/multitest.rs Show resolved Hide resolved
contracts/provider/external-staking/src/multitest.rs Outdated Show resolved Hide resolved
@jawoznia jawoznia force-pushed the f/value-ranges-external-staking branch from 0f6d8c7 to 309b8fd Compare August 22, 2023 13:35
@maurolacy maurolacy marked this pull request as ready for review August 22, 2023 14:04
@maurolacy
Copy link
Collaborator

maurolacy commented Aug 22, 2023 •
edited
Loading

Can be merged, but better if you review mine before merging this on top. Or change base to main, and merge it there.

@jawoznia jawoznia force-pushed the f/value-ranges-external-staking branch from f97f8dc to 70141fa Compare August 23, 2023 11:47
@jawoznia jawoznia merged commit 14a28b1 into f/value-ranges Aug 23, 2023
2 checks passed
@maurolacy maurolacy deleted the f/value-ranges-external-staking branch November 6, 2023 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maurolacy maurolacy maurolacy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jawoznia @maurolacy