-
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
Call SendRewards
from maintainer
#481
Conversation
7ef2860
to
1783556
Compare
83dd9f1
to
83a76a3
Compare
anker/src/lib.rs
Outdated
@@ -43,6 +43,10 @@ pub fn find_instance_address(anker_program_id: &Pubkey, solido_instance: &Pubkey | |||
Pubkey::find_program_address(&[solido_instance.as_ref()], anker_program_id) | |||
} | |||
|
|||
// TODO(ruuda): Use this below. |
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 started working on this, but it is going to be a pretty big and invasive change, so I split it off into a separate PR: #488
04118e5
to
6771008
Compare
fe0fcc8
to
fbb1174
Compare
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.
Looks mostly good, I just have a few questions
@@ -86,6 +89,7 @@ fn process_initialize( | |||
ANKER_STSOL_RESERVE_ACCOUNT, | |||
&[st_sol_reserve_account_bump_seed], | |||
]; | |||
msg!("Allocating account for stSOL reserve ..."); |
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.
Good to put some debug msgs! 👍
let instr = Box::new(spl_token::instruction::approve( | ||
accounts.spl_token.key, | ||
accounts.ust_reserve_account.key, | ||
accounts.authority_signer_key.key, |
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.
Note to self: do we check this authority_signer
?
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.
&wormhole_transfer_args.custody_key, | ||
accounts.custody_key.key, | ||
)?; | ||
check_wormhole_account( |
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.
Shouldn't we check this?!
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.
Very good catch! Fixed. I think what happened here is that I removed the custody key and custody signer key, because they are only used for native tokens, not wrapped tokens like UST, but I was a bit too eager and also removed this check in between that I shouldn’t have removed.
// 7cw4gLGZfH2rU5di5xeQbNZ1Nbc8D7i78jkxXtLUvnwyyZbha5E3Ew2izLjLTki56Ek1zQyZn2Ghb1tK4fWeMhE | ||
/// Test transaction that transfers UST on Solana to Terra. | ||
/// | ||
/// Based on this transaction: <https://explorer.solana.com/tx/5tSRA1CYLd51sjf7Dd2ZRkLspcqiR8NH51oTd3K34sNc3PZG9uF7euE2AHE95KurrcfKYf2sCQqsEbSRmzQq8oDg?cluster=devnet>. | ||
#[test] | ||
fn test_get_wormhole_instruction() { |
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.
New transaction test! Nice 👍
// there is a transfer of 0.000_000_010 SOL to _some_ account ... but | ||
// then the Wormhole call also transfers that amount. So it seems the | ||
// first one is a kind of tip? Can we skip it? | ||
// TODO(#489): // Also, we shouldn't transfer out of an account which may have more |
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.
🕵️
@@ -215,8 +215,8 @@ impl fmt::Display for ShowAnkerOutput { | |||
writeln!(f, "bSOL mint: {}", self.b_sol_mint)?; | |||
writeln!(f, "bSOL mint authority: {}", self.b_sol_mint_authority)?; | |||
writeln!(f, "bSOL supply: {}", self.b_sol_supply)?; | |||
writeln!(f, "Reserve authority: {}", self.st_sol_reserve)?; | |||
writeln!(f, "stSOL reserve address: {}", self.reserve_authority)?; | |||
writeln!(f, "Reserve authority: {}", self.reserve_authority)?; |
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.
Good catch
// transactions based on the same slot though? If we do, then the state | ||
// we observe should also be identical, and only one of the transactions | ||
// should succeed. | ||
let wormhole_nonce = (self.clock.slot & 0xffff_ffff) as u32; |
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.
Great 👍
cli/src/maintenance.rs
Outdated
|
||
let ust_amount = anker_state.ust_reserve_balance; | ||
let output = MaintenanceOutput::SendRewards { ust_amount }; | ||
println!("Trying to send rewards: {:?}", &output); |
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.
Is this a debug msg? Cause it can mess the tests (if we have one)
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.
Yes, good catch, that is a debugging leftover, removed now. We shouldn’t have bare println!
s in the CLI code because it will mess up the --output json
mode.
// Usually the maintainer is the only signer, but in some cases we | ||
// need to generate a new fresh keypair, which then also is a signer. | ||
let mut signers: Vec<&dyn Signer> = vec![config.signer]; | ||
for keypair in &maintenance_instruction.additional_signers { |
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.
That's how we get additional signers, nice!
This is incomplete; I don't yet know how to get all of these addresses. Will check the docs and ask the Wormhole developers. But for now this is a step forward to invoking Wormhole. As a drive-by fix, report the SellRewards count in the metrics too.
Thanks Fynn for helping with this.
This is a combination of some fixes and debug prints that me and Fynn pair-programmed. At this point we reach the Wormhole call, but there are also some debug changes that will need to be reverted later.
We started out calling transfer_native, but UST on Solana is not a native SPL token, it's a wrapped (by Wormhole) token.
Instead of making it look like the Ethereum test transaction, model it after the Solana->Terra UST transaction that we did, because that one is the right use case: the chain and token type (wrapped) match.
I introduced this type but did not adjust the tests for it. Now they compile and pass again.
See also the added comment for the motivation for this.
Thanks Fynn for catching this. I think this was an oversight when I removed the custody key and custody signer key (because these are not used for wrapped tokens, only for native tokens) but I missed that this check in between had to stay.
c5f3d30
to
3fb4bac
Compare
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!
This modifies the maintainer daemon to call
SendRewards
in case there is UST in the UST reserve.Also contains a number of fixes that @enriquefynn and me pair-programmed while trying to get this to work on Solana devnet.