-
Notifications
You must be signed in to change notification settings - Fork 4
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
Solana program #113
Conversation
[WIP] Solana side using classic token program
Ok(()) | ||
} | ||
|
||
pub fn init_transfer_bridged( |
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 it possible to combine these two methods init_transfer_bridged
& init_transfer_native
into 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.
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
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.
We should try to combine them, and add an if/else flow. The same for the fin_transfer
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.
We can make vault an optional account but it will be harder to read the history in the block explorer
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.
This is done now
solana/bridge_token_factory/programs/bridge_token_factory/src/lib.rs
Outdated
Show resolved
Hide resolved
impl Payload for DeployTokenResponse { | ||
type AdditionalParams = (); | ||
|
||
fn serialize_for_near(&self, _params: Self::AdditionalParams) -> Result<Vec<u8>> { |
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.
Why the custom serialization is used everywhere instead of borsh macros?
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.
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
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.
outgoing payload will be read by near so in case you need for something near specific
...ken_factory/programs/bridge_token_factory/src/instructions/user/finalize_transfer_bridged.rs
Outdated
Show resolved
Hide resolved
// 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(); |
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.
Why this code is duplicate the wormhole_cpi
?
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.
Not sure what you mean here
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 mean this code looks similar to this one
Line 128 in 3067c16
let fee = self.wormhole_bridge.fee(); |
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.
Was required for the previous version of the code. Now may be unified
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.
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)] |
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.
Why the #[repr(C)]
is used for this struct?
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.
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; |
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.
Isn't the bitvec crate too heavy for on-chain?
Example of simple implementation https://gist.github.com/karim-en/0e11b634ef049ebfd18de491644ecb67
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.
It must be POD struct for zero_copy
)?; | ||
} | ||
} | ||
#[cfg(not(feature = "idl-build"))] |
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.
What is the propose of this feature?
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.
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.
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; |
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.
What is the propose of this refund logic?
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.
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; |
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.
Can you please add a comment where the extra +8 came from.
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.
Anchor discriminator
Downgrade anchor to release and rename ixes in ts code
a if self.mint.mint_authority.contains(&a) => {} | ||
_ => return err!(ErrorCode::Unauthorized), | ||
} | ||
(metadata_override.name, metadata_override.symbol) |
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.
@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.
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.
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.
No, this is optional. Metadata is retrieved automatically but admin can override
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.
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.
...na/bridge_token_factory/programs/bridge_token_factory/src/state/message/finalize_transfer.rs
Outdated
Show resolved
Hide resolved
solana/bridge_token_factory/programs/bridge_token_factory/Cargo.toml
Outdated
Show resolved
Hide resolved
* Solana: implement native fee and SOL transfers * Add rent for SOL vault * Fix payloads
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