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

Solana program #113

Merged
merged 37 commits into from
Nov 17, 2024
Merged

Solana program #113

merged 37 commits into from
Nov 17, 2024

Conversation

kiseln
Copy link
Contributor

@kiseln kiseln commented Nov 12, 2024

Solana program for the OmniBridge. The only functionality that's missing is providing fee in native tokens. Also, logging metadata for Solana-first tokens will probably be modified once we finalize the standard on the Near side

Typescript cli for interacting is currently broken. We will re-evaluate whether it's needed

Ok(())
}

pub fn init_transfer_bridged(
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it possible to combine these two methods init_transfer_bridged & init_transfer_native into one?

Copy link
Contributor Author

@kiseln kiseln Nov 13, 2024

Choose a reason for hiding this comment

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

We need to provide a slightly different set of accounts for these methods because in one case we burn the tokens, while in the other - lock them. I'm not sure if it's possible to combine but seems like it's not the best practice.
@aankor perhaps may have more comments

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should try to combine them, and add an if/else flow. The same for the fin_transfer

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can make vault an optional account but it will be harder to read the history in the block explorer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is done now

impl Payload for DeployTokenResponse {
type AdditionalParams = ();

fn serialize_for_near(&self, _params: Self::AdditionalParams) -> Result<Vec<u8>> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why the custom serialization is used everywhere instead of borsh macros?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For incoming payloads we need to serialize them according to Anchor rules, so we need a custom serialization to compare it to the message signed on NEAR.

For outgoing payloads we can switch to macros and create a separate struct for each message

Copy link
Collaborator

Choose a reason for hiding this comment

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

outgoing payload will be read by near so in case you need for something near specific

// If Wormhole requires a fee before posting a message, we need to
// transfer lamports to the fee collector. Otherwise
// `wormhole::post_message` will fail.
let fee = self.bridge.fee();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this code is duplicate the wormhole_cpi?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you mean here

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean this code looks similar to this one

Copy link
Collaborator

Choose a reason for hiding this comment

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

Was required for the previous version of the code. Now may be unified

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was required for the previous version of the code. Now may be unified

I suppose we can use wormhole cpi in intialize, just need to convert config account to init_if_needed


#[cfg(not(feature = "idl-build"))]
#[account(zero_copy(unsafe))]
#[repr(C)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why the #[repr(C)] is used for this struct?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Required by zero_copy as I know. We need for zero_copy here otherwise it will copy the array (but we need for one bit only)

use anchor_lang::system_program::transfer;
use anchor_lang::system_program::Transfer;
#[cfg(not(feature = "idl-build"))]
use bitvec::array::BitArray;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't the bitvec crate too heavy for on-chain?
Example of simple implementation https://gist.github.com/karim-en/0e11b634ef049ebfd18de491644ecb67

Copy link
Collaborator

Choose a reason for hiding this comment

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

It must be POD struct for zero_copy

)?;
}
}
#[cfg(not(feature = "idl-build"))]
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the propose of this feature?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is turned on for the second compilation used to generate IDL

)?;
} else {
// compensate for the account creation
let compensation = current_rent_reserve_lamports - expected_rent_reserve_lamports;
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the propose of this refund logic?

Copy link
Collaborator

@aankor aankor Nov 15, 2024

Choose a reason for hiding this comment

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

When the nonce goes to the new account the rent for whole account creation was already paid by anchor so we must compensate from reserve prepared by previous existent account users

pub const USED_NONCES_PER_ACCOUNT: u32 = 1024;

#[constant]
pub const USED_NONCES_ACCOUNT_SIZE: u32 = 8 + (USED_NONCES_PER_ACCOUNT + 7) / 8;
Copy link
Collaborator

@karim-en karim-en Nov 12, 2024

Choose a reason for hiding this comment

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

Can you please add a comment where the extra +8 came from.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Anchor discriminator

Downgrade anchor to release and rename ixes in ts code
@karim-en karim-en requested review from r-near and karim-en November 13, 2024 11:10
a if self.mint.mint_authority.contains(&a) => {}
_ => return err!(ErrorCode::Unauthorized),
}
(metadata_override.name, metadata_override.symbol)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@kiseln
Do I understand correctly that the metadata should be passed manually by the admin of the contract?
If yes, then this flow overcomplicate the deploy process on Solana, the name and symbol can be empty, and the metadata can be set by the DAO on the NEAR side.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this is optional. Metadata is retrieved automatically but admin can override

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I mean there is no sense to provide this functionality, it is much easier to set and override the metadata on NEAR side by MetadataManager Role.

* Solana: implement native fee and SOL transfers

* Add rent for SOL vault

* Fix payloads
@karim-en karim-en merged commit ffc8cd3 into main Nov 17, 2024
4 checks passed
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.

3 participants