Conversation
…sign queue once fail or success
|
This is still work in progress, will address comments when it's in more stable shape (things are connected to one another) |
|
This is working on top of Felippe's PR that changes Solana indexer to parse CPI events: #439. This PR has implemented:
This PR has not implemented:
|
|
Next steps:
|
ChaoticTempest
left a comment
There was a problem hiding this comment.
did an initial review, will come back to do some more later, but looks promising so far
|
how are we testing this so far? Feels like this can easily regress if we don't have an integration test |
jakmeier
left a comment
There was a problem hiding this comment.
Great progress!
There are lots of moving parts in this and I believe the first priority is to have something that just about works, without breaking things. We will need to iterate on it to make it more general for different chain to chain directions, more robust against error cases, refactor to make it more testable, add test cases, and potentially optimize async flows for performance. But I think we shouldn't wait too much before merging a first version of this to develop.
| tracing::info!("found solana event: {:?}", self); | ||
| if self.deposit == 0 { | ||
| tracing::warn!("deposit is 0, skipping sign request"); | ||
| return Err(anyhow::anyhow!("deposit is 0")); |
There was a problem hiding this comment.
nit: instead of returning Err(anyhow!()) there is also the convenience macro bail!() that does is slightly more succinctly.
https://docs.rs/anyhow/latest/anyhow/macro.bail.html
This macro is equivalent to return Err(anyhow!($args...))
928a121 to
399d50c
Compare
| pub algo: String, | ||
| pub dest: String, | ||
| pub params: String, | ||
| pub explorer_deserialization_format: u8, |
There was a problem hiding this comment.
What is explorer in this context?
There was a problem hiding this comment.
i followed the naming from fake mpc. @esaminu might know more detail about it, i'm happy to change it
There was a problem hiding this comment.
@esaminu please, take a look at this question even if we merge this.
|
for the most part, it looks good to me, but let's wait on others to see if they notice anything else |
volovyks
left a comment
There was a problem hiding this comment.
I've resolved most of our conversations (feel free to reopen), and created a couple of issues for things we should fix or look into in the future. Let's merge this one and iterate.
This code now passes the complete successful deposit flow (https://github.com/sig-net/solana-contract-examples/blob/main/contract/tests/sign-respond-erc20.ts#L322).
Next step:
Understand and see how to set up testing all the other cases in https://github.com/sig-net/solana-contract-examples/blob/main/contract/tests/sign-respond-erc20.ts.
I think we could start reviewing.