-
Notifications
You must be signed in to change notification settings - Fork 308
[remote-executor] Isolate wormhole-solana crate #843
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ |
cd41728 to
bcf423e
Compare
| @@ -0,0 +1,2 @@ | |||
| [toolchain] | |||
| channel = "1.66.1" | |||
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.
Drive-by pin toolchain
| std::env::current_dir() | ||
| .unwrap() | ||
| .join(Path::new("../../target/deploy/remote_executor.so")), | ||
| ); |
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.
Newer versions of solana can't find the binary with the previous logic
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.
Can you explain why? Inherently suspicious of needing relative paths.
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.
In previous versions (1.13.x) when you ran cargo test-bpf, it would set an environment variable BPF_OUT_DIR and that's how the code would find the binary.
New versions (1.14.x) don't seems to set it, so find_file fails.
Maybe it has to do with them trying to migrate to test-sbf and deprecating test-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.
Yeah I recall the BPF_OUT_DIR issues as well. Can you mention what might be the reason it's not doing it before we merge this? Don't worry if it takes too much time you can merge over this comment, it would just be nice to know what changed here for other solana related work.
| - name: Install Solana | ||
| run: | | ||
| sh -c "$(curl -sSfL https://release.solana.com/stable/install)" | ||
| sh -c "$(curl -sSfL https://release.solana.com/v1.14.18/install)" |
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 had to bump this
| overflow-checks = true | ||
|
|
||
| [patch.crates-io] | ||
| serde_wormhole = { git = "https://github.com/wormhole-foundation/wormhole", tag = "v2.17.1" } |
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 patch.crate-io wormhole thing is getting out of control, wormhole needs to fix
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.
Agreed, reasonable fix for now though. Let's ask for publishing.
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.
Attester cargo lock updated since attester governance cli uses remote-executor
| std::env::current_dir() | ||
| .unwrap() | ||
| .join(Path::new("../../target/deploy/remote_executor.so")), | ||
| ); |
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.
Can you explain why? Inherently suspicious of needing relative paths.
| anchor-lang = {version = "0.25.0", features = ["init-if-needed"]} | ||
| wormhole-solana = { git = "https://github.com/guibescos/wormhole", branch = "reisen/sdk-solana"} | ||
| wormhole-core = { git = "https://github.com/guibescos/wormhole", branch = "reisen/sdk-solana"} | ||
| wormhole-solana = { git = "https://github.com/guibescos/wormhole-solana", rev="f14b3b54c1e37e1aaf8c2ac2a5e236832ffdb3c2"} |
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.
We should really get away from this, maybe we can chat about what needs to be upstreamed. We're running into this in pythnet too. I believe this was based on my original PR that had plans to revive, we should push for it if possible.
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 a temporary home.
Two options from here :
- Its own repo (maybe switch from my personal github to wormhole or pyth org)
- Make it part of
sdk/rust/in the wormhole monorepo
|
|
||
| let vaa = VAA::from_bytes(vaa_bytes.clone())?; | ||
| let vaa: Vaa<&RawMessage> = serde_wormhole::from_slice(&vaa_bytes)?; | ||
| let (header, body): (Header, Body<&RawMessage>) = vaa.into(); |
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.
Nice nice nice.
| overflow-checks = true | ||
|
|
||
| [patch.crates-io] | ||
| serde_wormhole = { git = "https://github.com/wormhole-foundation/wormhole", tag = "v2.17.1" } |
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.
Agreed, reasonable fix for now though. Let's ask for publishing.
The goal of this PR is :
wormhole-sdkwormhole-solana(that wormhole doesn't have) to its own repo and update it to usewormhole-sdkWormhole team needs to :
wormhole-solana