feat: solana resigning

[ramizhasan111]

Pull Request

Closes: PRO-1524

Checklist

Please conduct a thorough self-review before opening the PR.

• I am confident that the code works.
• I have written sufficient tests.
• I have written and tested required migrations.
• I have updated documentation where appropriate.

Summary

Please include a succinct description of the purpose and content of the PR. What problem does it solve, and how? Link issues, discussions, other PRs, and anything else that will help the reviewer.

Non-Breaking changes

If this PR includes non-breaking changes, select the non-breaking label. On merge, CI will automatically cherry-pick the commit to a PR against the release branch.

[albert-llimos]

I think it'd be good to have a test for the Solana resigning if possible, especially given that we are not rebuilding the transaction but rather replacing some accounts.

[codecov]

Codecov Report

Attention: Patch coverage is 93.23308% with 9 lines in your changes missing coverage. Please review.

Project coverage is 71%. Comparing base (ecf4c58) to head (a4f1554).
Report is 1 commits behind head on main.

Files	Patch %	Lines
state-chain/pallets/cf-broadcast/src/lib.rs	64%	2 Missing and 2 partials ⚠️
state-chain/chains/src/sol/api.rs	78%	2 Missing ⚠️
state-chain/runtime/src/chainflip.rs	95%	2 Missing ⚠️
state-chain/cf-integration-tests/src/solana.rs	99%	1 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff          @@
##            main   #5124   +/-   ##
=====================================
- Coverage     71%     71%   -0%     
=====================================
  Files        460     460           
  Lines      81931   81896   -35     
  Branches   81931   81896   -35     
=====================================
- Hits       58431   58341   -90     
- Misses     20408   20454   +46     
- Partials    3092    3101    +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[syan095]

Might be worth it to write an integration test just to be safe.

[ramizhasan111]

I think it'd be good to have a test for the Solana resigning if possible, especially given that we are not rebuilding the transaction but rather replacing some accounts.

tried to write a unit test but realized that it is not possible. will write an integration test.

[dandanlen]

I left some comments.

Also, reading this, I'm not sure why the signer was added inside the ApiCall instead of adding it next to the ApiCall in storage ((signer, api_call)) - might have made things simpler?

[ramizhasan111]

Also, reading this, I'm not sure why the signer was added inside the ApiCall instead of adding it next to the ApiCall in storage ((signer, api_call)) - might have made things simpler?

would have been much simpler, yes (especially the migrations 😢 ). I wanted it to be a part of the apicall type because

1. it couples the signer info tightly with the call and its signature. We store the signature in the Apicall and it makes sense to store the signer with it so they cant get out of sync and have to be changed together.
2. the signer information is fundamental to the Apicall and we might need it elsewhere in the future while using the Apicall type

[dandanlen]

Also, reading this, I'm not sure why the signer was added inside the ApiCall instead of adding it next to the ApiCall in storage ((signer, api_call)) - might have made things simpler?

would have been much simpler, yes (especially the migrations 😢 ). I wanted it to be a part of the apicall type because

1. it couples the signer info tightly with the call and its signature. We store the signature in the Apicall and it makes sense to store the signer with it so they cant get out of sync and have to be changed together.

2. the signer information is fundamental to the Apicall and we might need it elsewhere in the future while using the Apicall type

tbh. I don't really agree with this (re. 1. we have already seen that the call.signer can get out of sync with the values in the inner tx, see above, and 2. is quite speculative) but i suppose now it's done, we can live with it.

[ramizhasan111]

tried to write an integration test that tests the edge case where solana resigning is required but realized that that is also not possible easily because there is no mock cfe functionality of submitting/failing tx broadcasts. Insteadm I wrote an integration test that functions as a unit test for the requires_signature_refresh logic.

[albert-llimos]

tried to write an integration test that tests the edge case where solana resigning is required but realized that that is also not possible easily because there is no mock cfe functionality of submitting/failing tx broadcasts. Insteadm I wrote an integration test that functions as a unit test for the requires_signature_refresh logic.

Makes sense. I'll try to add some extra checks to ensure that we obtain a valid transaction, let's see if I can make that work.