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

Implement AsyncAurora contract #489

Closed
wants to merge 15 commits into from
Closed

Conversation

karim-en
Copy link
Contributor

Implement AsyncAurora contract to support Aurora Cross Contract Calls

Copy link
Contributor

@sept-en sept-en left a comment

Choose a reason for hiding this comment

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

Some minor comments while it's still draft. More comments will be added when it's not a draft anymore

promise_desc.gas,
);

promises.push(promise.clone());
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a redundant clone, let's avoid this

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 77836d8

Comment on lines 15 to 24
#[ext_contract(ext_bridge_token_factory)]
pub trait ExtBridgeTokenFactory {
#[result_serializer(borsh)]
fn finish_withdraw(
&self,
#[serializer(borsh)] amount: Balance,
#[serializer(borsh)] recipient: AccountId,
) -> Promise;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems this is redundant and left here after the migration from the other repo.

Suggested change
#[ext_contract(ext_bridge_token_factory)]
pub trait ExtBridgeTokenFactory {
#[result_serializer(borsh)]
fn finish_withdraw(
&self,
#[serializer(borsh)] amount: Balance,
#[serializer(borsh)] recipient: AccountId,
) -> Promise;
}

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 acbf0b0

combinator: Option<CombinatorDescription>,
}

fn parse_promises(input: String) -> Vec<PromiseDescription> {
Copy link
Contributor

Choose a reason for hiding this comment

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

This rather should be the part of the appropriate trait.

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 acbf0b0

@mfornet mfornet self-requested a review April 18, 2022 09:29
@joshuajbouw
Copy link
Contributor

Just be aware, that we need to have DAO approval for this.

Copy link
Contributor

@sept-en sept-en left a comment

Choose a reason for hiding this comment

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

Added some more comments and questions for discussions. Also, please add README for async-aurora with appropriate references to cross-contract-call and native token connector design discussions.

string memory accountId,
string memory method,
uint128 arg,
string memory gas
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we use gas as a string even in tests?

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 d4e17fc

}
}

fn submit(context: &TestContext, input: Vec<u8>) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make the meaning clear

Suggested change
fn submit(context: &TestContext, input: Vec<u8>) {
fn call_aurora_async_submit(context: &TestContext, input: Vec<u8>) {

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 bfe5844

.assert_success();
}

fn caller_simple_call(context: &TestContext, method: String, arg: u128) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add this prefix instead of caller in all these test functions

Suggested change
fn caller_simple_call(context: &TestContext, method: String, arg: u128) {
fn exec_testasync_simple_call(context: &TestContext, method: String, arg: u128) {

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 f004ca6

Comment on lines +62 to +97
let promises_str = ethabi::decode(&[ethabi::ParamType::String], output.as_slice())
.unwrap()
.pop()
.unwrap()
.to_string()
.unwrap();
let promises_desc = parse_promises(promises_str);
let mut promises: Vec<Promise> = Vec::with_capacity(promises_desc.len());
for promise_desc in promises_desc {
let mut promise = Promise::new(promise_desc.target.clone())
.function_call(
promise_desc.method_name.clone(),
promise_desc.arguments.clone(),
0,
promise_desc.gas,
)
.as_return();

if let Some(combinator) = &promise_desc.combinator {
match combinator.combinator_type {
CombinatorType::And => {
promise = promises[combinator.promise_index as usize]
.clone()
.and(promise);
}
CombinatorType::Then => {
promise = promises[combinator.promise_index as usize]
.clone()
.then(promise);
}
}
}

promises.push(promise);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

For a discussion: do we want to have lazy-promise execution? This way we may need to ask the user to attach enough ETH to pay for the storage needed to store promises in async.aurora before they are executed. Also, we will need to have a precompile allowing the user to execute unfinished promises that are already stored in async.aurora in some mapping (execution_id: Vec<Promise>)

use near_sdk::borsh::{self, BorshDeserialize, BorshSerialize};
use near_sdk::{assert_self, env, ext_contract, near_bindgen, Gas, Promise};

pub const GAS_RESERVED_FOR_CURRENT_CALL: Gas = 20_000_000_000_000;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggesting changing the name. Also, are you sure 20 TGas will be enough? What's the real usage value?

Suggested change
pub const GAS_RESERVED_FOR_CURRENT_CALL: Gas = 20_000_000_000_000;
pub const GAS_RESERVED_FOR_SUBMIT: Gas = 20_000_000_000_000;

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 bfe5844

Comment on lines 2 to 7
name = "async-aurora"
version = "0.1.0"
authors = ["Aurora <hello@aurora.dev>"]
edition = "2021"
publish = false

Copy link
Contributor

Choose a reason for hiding this comment

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

let's also add the description.

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 f004ca6

near_account_to_evm_address(predecessor_account_id.as_bytes())
}

impl Precompile for AsyncRouter {
Copy link
Contributor

Choose a reason for hiding this comment

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

Another idea would be to implement AsyncRouter as a regular Solidity contract and create a precompile that will call that router contract. The router contract could be deployed using a separate function (e.g. deploy_router_contract().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is the point of contention, I also preferred the regular solidity contract, but I got request to implement it as precompile

@karim-en karim-en marked this pull request as ready for review April 27, 2022 00:05
@joshuajbouw
Copy link
Contributor

Please keep this as a draft. There is still a lot to discuss about this.

@joshuajbouw joshuajbouw marked this pull request as draft April 27, 2022 08:40
* Add description
* Rename call method
@karim-en karim-en force-pushed the cross-contracts-call-async branch from 7a91456 to f004ca6 Compare May 12, 2022 00:59
Comment on lines +1 to +7
use near_sdk::borsh::{self, BorshDeserialize, BorshSerialize};

pub type EthAddress = [u8; 20];
pub type RawU256 = [u8; 32];
/// Wei compatible Borsh-encoded raw value to attach an ETH balance to the transaction
pub type WeiU256 = [u8; 32];

Copy link
Contributor

Choose a reason for hiding this comment

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

NB: this file is being replicated twice in both etc/async-aurora-test and etc/async-aurora. Moreover, it shouldn't be duplicated there, rather should be separated into a separate lib in the repo itself.

@bharath-123
Copy link

Would this also cover querying a NEAR contract? It is great that we can call a NEAR contract, but i would like to understand if this feature would allow us to query a NEAR contract method too?

@birchmd
Copy link
Member

birchmd commented Sep 7, 2022

Closing this as cross-contract calls were implemented in #560

@birchmd birchmd closed this Sep 7, 2022
@aleksuss aleksuss deleted the cross-contracts-call-async branch October 16, 2023 10:33
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.

5 participants