-
Notifications
You must be signed in to change notification settings - Fork 43
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
Add Solana Program Library as a submodule #90
Conversation
This does make one test fail:
Edit: This is definitely caused by the fix for the fee computation. I’ll fix the test. Edit: Test fixed. |
@@ -4,7 +4,7 @@ on: | |||
push: | |||
branches: [ main ] | |||
pull_request: | |||
branches: [ main ] | |||
branches: '*' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed CI was not running for this pull request, because I set the base to add-multisig-tests
(it depends on #62). This fixes that.
638fef4
to
fa81925
Compare
35365ae
to
66b5ff2
Compare
@ruuda the tests seems to be failing still |
After running |
It seems we need to run |
Hmm, so the difference was in running |
@@ -93,6 +96,8 @@ async fn test_successful_update_balance() { | |||
.await; | |||
assert!(error.is_none()); | |||
|
|||
// Transfer EXTRA_STAKE_AMOUNT Lamports to every validator outside of Lido |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comments, I'll be more prolific about it ^^
sudo apt-get install -y libudev-dev | ||
sh -c "$(curl -sSfL https://release.solana.com/v1.6.7/install)" | ||
export PATH="$HOME/.local/share/solana/install/active_release/bin:$PATH" | ||
cargo build-bpf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to test also the solana-program-library, is there a way to call only Lido's tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cargo test-bpf --manifest-path program/Cargo.toml
seems to do the trick, will update that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes tests to the solana-program-library submodule, I think it would be better to avoid these tests
You would have to mark the tests with #[ignore] in the submodule but I don't think that is viable. |
Maybe also delete |
Aaah, that might be where it came from! Good catch, removed. |
Solido depends on the SPL stake pool program, which is not present on a local testnet by default, so we need to build it from source. To be able to do that reliably in the tests, pull in the SPL as a submodule here, so we can depend on its stake pool program. The specific commit adds one patch on top of the upstream repository, which removes the Cargo workspace in that repository, to prevent it from interfering with our workspace. Aside from helping to test on a localnet, having a pinned version of the SPL stake pool program is also useful if we want to deploy our own version of the stake pool: we can pin it, and upgrade at our own pace. But we can still pull in the latest changes from upstream in the submodule, there is no difficult patch backporting process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I rebased now that #75 has been merged. I had to fix one test that was added, the fix is 36d6902. @enriquefynn if this looks good to you I will go ahead and merge. |
After pulling in a new version of the SPL stake pool, which fixes the way it computes rewards, we need to update the test to match, because the number of reward tokens is indeed different.
Without this, if you open a stacked pull request (against the branch of an existing pull request), it will not run CI for that pull request.
For some reason, the "multisig" mod in the crate was conflicting with the "multisig" crate name itself. I'm not sure why this suddenly started happening now, but renaming the mod inside resolved the problem. This updates the submodule to the commit that includes that rename.
The test that depends on the SPL stake pool program fails without first running "cargo build-bpf"; the fee computation is wrong. I'm not sure why this happens, it's as if it uses an older version of the SPL stake pool program, but I don't know where it gets that from. Either way, running "cargo build-bpf" before running the tests makes the tests pass ...
So the previous commit still fails on CI ... what stood out is that the command completed quickly, I think it did not actually build anything. So let's try after a clean.
I think this might be the trigger for "cargo test-bpf" to use the wrong version of the stake pool program? It runs from the wrong directory?
We now pull in the SPL stake pool through a submodule and build it; the artifact should not live in the repository.
Solido depends on the SPL stake pool program, which is not present on a local testnet by default, so we need to build it from source. To be able to do that reliably in the tests, pull in the SPL as a submodule here, so we can depend on its stake pool program.
The specific commit adds one patch on top of the upstream repository, which removes the Cargo workspace in that repository, to prevent it from interfering with our workspace.
Aside from helping to test on a localnet, having a pinned version of the SPL stake pool program is also useful if we want to deploy our own version of the stake pool: we can pin it, and upgrade at our own pace. But we can still pull in the latest changes from upstream in the submodule, there is no difficult patch backporting process.
The version of the SPL that we pull in is more recent than the one that we depended on previously. In particular it includes a fix for fee distribution in the stake pool.