Skip to content
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

feat: add solana ampd implementation #744

Open
wants to merge 151 commits into
base: main
Choose a base branch
from

Conversation

eloylp
Copy link
Contributor

@eloylp eloylp commented Jan 13, 2025

At Eiger, we are implementing the integration between Axelar and Solana.

This PR introduces the necessary logic for the ampd component, including:

  • The Solana message verifier handler.
  • The Solana verifier set verifier handler.
  • The logic required to validate Solana addresses.

Both verifier components share the same Solana transaction fetch and log parsing logic, which facilitates identifying event sequences within Solana programs execution outputs.

Todos

  • Unit tests
  • Manual tests (pending - waiting Axelar contract deployment approval)
  • Documentation
  • Connect epics/issues

Steps to Test

  1. Configure ampd with an updated ampd.toml file, pointing to a local tofnd setup.
  2. tofnd must be up and running.
  3. Compile and start the ampd daemon.
  4. Execute the memo program flow, which sends a message from Evm to Solana and viceversa. Scripts can be found at solana-axelar-scripts

Expected Behaviour

It should forward GMP messages from one chain to the another, passing them through the axelar devnet network.

Other Notes

Manual testing is missing because we are waiting for Axelar contract approval.

eloylp and others added 30 commits February 6, 2024 20:44
* Add Solana account parsing tools.
* Add solana verification functionality

* Add unit tests to Solana message verifier

* Refactor

* Solana msg verifier reshape (#4)

* remove comment

* Use Solana types from Solana crates

We were using hand crafted Solana types for interacting with RPC.

* refactor: make clippy happy

* Add some traces to Solana msg verifier

* Remove outdated todos

* refactor: re-use already calculated variable

* Clarify test code usage

* Solana msg verifier, try till one program log matches gw event

* Add note to axelar to review when in PR

* fail if tx_id is not found in signatures

* Always fail on chain at handler level

* Handle errors in case a tx cannot be found (finish the handler)

* Remove commented code

* Use original type from solana-axelar repo

* Ensure that we verify sender address in sol msg verifier

* Do not panic on PartialEq

* refactor: clippy

* remove no longer used code

* Add more tests definition to solana msg verify iteration logic

---------

Co-authored-by: eloylp <[email protected]>
Were only intended during development to understand the API.
Co-authored-by: tilacog <[email protected]>
Co-authored-by: tilacog <[email protected]>
Co-authored-by: tilacog <[email protected]>
@eloylp eloylp requested a review from a team as a code owner January 13, 2025 13:11
bugfix on event parsing
Comment on lines 139 to 140
let poll_id_str: String = poll_id.into();
let source_chain_str: String = source_chain.into();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be cleaner to just do this inline

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed at 3a5d750

Comment on lines 120 to 121
};
if !participants.contains(&self.verifier) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
};
if !participants.contains(&self.verifier) {
};
if !participants.contains(&self.verifier) {

Copy link
Contributor Author

@eloylp eloylp Feb 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed at f7fd35a

Comment on lines 19 to 30
GatewayEvent::CallContract(event) => (
&event.sender_key,
&event.payload_hash,
&event.destination_chain,
&event.destination_contract_address,
),
GatewayEvent::CallContractOffchainData(event) => (
&event.sender_key,
&event.payload_hash,
&event.destination_chain,
&event.destination_contract_address,
),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might want to add some comments here about why there are two events

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed at 028f434

}
};

// pare the events
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// pare the events
// compare the events

Copy link
Contributor Author

@eloylp eloylp Feb 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed at 4b3c1fb

where
F: Fn(&GatewayEvent) -> bool,
{
// the event idx cannot be larger than usize
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why? also we might want to fail earlier in this case, there's no point to even make the RPC call

if !participants.contains(&self.verifier) {
return Ok(vec![]);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should check the source chain

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am assuming ™️ (correct me if am wrong) each chain deploys it's own voting verifier cosmwasm contract. If that's true, wouldn't this check be enough ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't see the source chain check in any implementation apart from evm

if !participants.contains(&self.verifier) {
return Ok(vec![]);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should check the source chain

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's follow conversation here: #744 (comment)

@@ -69,7 +69,7 @@ impl PartialEq<ContractEventBody> for VerifierSetConfirmation {
}
}

pub fn verify_message(gateway_address: &ScAddress, tx_receipt: &TxResponse, msg: &Message) -> Vote {
pub fn verify_message(gateway_address: &ScAddress, tx_receipt: &TxResponse, msg: &Message) -> Vote {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub fn verify_message(gateway_address: &ScAddress, tx_receipt: &TxResponse, msg: &Message) -> Vote {
pub fn verify_message(gateway_address: &ScAddress, tx_receipt: &TxResponse, msg: &Message) -> Vote {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed at f7fd35a

Comment on lines 44 to 53
AddressFormat::Base58Solana => {
const SOLANA_PUBKEY_LEN: usize = 32;

let pubkey_vec = bs58::decode(address)
.into_vec()
.change_context(Error::InvalidAddress(address.to_string()))?;
if pubkey_vec.len() != SOLANA_PUBKEY_LEN {
bail!(Error::InvalidAddress(address.to_string()))
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where is this being used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shouldn't be part of this branch (unrelated changes), we removed it at 00ed5e3

ampd/README.md Outdated
cosmwasm_contract = "axelar1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqecnww6"
chain_name = "solana"
chain_rpc_url = "https://api.devnet.solana.com"
max_tx_cache_entries = 6
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct? Where is this handled?

Copy link
Contributor Author

@eloylp eloylp Feb 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was part of a previous, already removed implementation. Fixed at a28db9d by removing it as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants