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

Girazoki runtime 901 moonbase alpha approval fix #974

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 14 additions & 19 deletions precompiles/assets-erc20/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,6 @@ where
context: &Context,
) -> EvmResult<PrecompileOutput> {
let mut gasometer = Gasometer::new(target_gas);
gasometer.record_cost(RuntimeHelper::<Runtime>::db_write_gas_cost())?;
gasometer.record_log_costs_manual(3, 32)?;

// Parse input.
Expand All @@ -291,40 +290,38 @@ where
Runtime::account_to_asset_id(execution_address)
.ok_or(error("non-assetId address"))?;

let caller: Runtime::AccountId =
Runtime::AddressMapping::into_account_id(context.caller);
let origin = Runtime::AddressMapping::into_account_id(context.caller);

let spender: Runtime::AccountId = Runtime::AddressMapping::into_account_id(spender);

// Dispatch call (if enough gas).
// We first cancel any existing approvals
// Since we cannot check storage, we need to execute this call without knowing whether
// another approval exists already.
// But we know that if no approval exists we should get "Unknown"
// Allowance() should be checked instead of doing this Result matching
let used_gas = match RuntimeHelper::<Runtime>::try_dispatch(
<<Runtime as frame_system::Config>::Call as Dispatchable>::Origin::root(),
pallet_assets::Call::<Runtime, Instance>::force_cancel_approval {
Some(origin.clone()).into(),
pallet_assets::Call::<Runtime, Instance>::cancel_approval {
id: asset_id,
owner: Runtime::Lookup::unlookup(caller),
delegate: Runtime::Lookup::unlookup(spender.clone()),
},
gasometer.remaining_gas()?,
) {
Ok(gas_used) => Ok(gas_used),
Err(ExitError::Other(e)) => {
// One DB read for checking the approval did not exist
if e.contains("Unknown") {
Ok(RuntimeHelper::<Runtime>::db_read_gas_cost())
} else {
Err(ExitError::Other(e))
}
}
// ExitError::Other is only returned if the dispatchable fails with an error
// We cannot format the error in wasm
// In our case, we know that if cancel_approval fails approve_transfer will also
// fail in all cases except the one we are interested on: that it does not exist
// a previous approval. In this case we still want to continue, so it is safe
// to do this
// TODO: Once 0.9.12 is here, we should be able to only call cance_approval if
// such approval exists.
Err(ExitError::Other(_e)) => Ok(2 * RuntimeHelper::<Runtime>::db_read_gas_cost()),
// Any other error can be returned
Err(e) => Err(e),
}?;
gasometer.record_cost(used_gas)?;

let origin = Runtime::AddressMapping::into_account_id(context.caller);

// Dispatch call (if enough gas).
let used_gas = RuntimeHelper::<Runtime>::try_dispatch(
Some(origin).into(),
Expand Down Expand Up @@ -413,8 +410,6 @@ where
context: &Context,
) -> EvmResult<PrecompileOutput> {
let mut gasometer = Gasometer::new(target_gas);
gasometer.record_cost(RuntimeHelper::<Runtime>::db_read_gas_cost())?;
gasometer.record_cost(RuntimeHelper::<Runtime>::db_write_gas_cost())?;
Comment on lines -416 to -417
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They were wrong. I think they were left-overs from when we were using a local approval storage and not the pallet one. Now the db writes assocaited with the pallet-storage should be returned by the dispatch call itself

gasometer.record_log_costs_manual(3, 32)?;

// Parse input.
Expand Down
2 changes: 1 addition & 1 deletion precompiles/assets-erc20/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -727,7 +727,7 @@ fn transfer_from_non_incremental_approval() {
Some(Ok(PrecompileOutput {
exit_status: ExitSucceed::Returned,
output: Default::default(),
cost: 115329756u64,
cost: 114357756u64,
logs: LogsBuilder::new(Account::AssetId(0u128).into())
.log3(
SELECTOR_LOG_APPROVAL,
Expand Down
3 changes: 2 additions & 1 deletion runtime/moonbase/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
spec_name: create_runtime_str!("moonbase"),
impl_name: create_runtime_str!("moonbase"),
authoring_version: 3,
spec_version: 0900,
spec_version: 0901,
impl_version: 0,
apis: RUNTIME_API_VERSIONS,
transaction_version: 2,
Expand Down Expand Up @@ -1382,6 +1382,7 @@ impl Contains<Call> for NormalFilter {
pallet_assets::Call::transfer_keep_alive { .. } => true,
pallet_assets::Call::approve_transfer { .. } => true,
pallet_assets::Call::transfer_approved { .. } => true,
pallet_assets::Call::cancel_approval { .. } => true,
_ => false,
},
_ => true,
Expand Down
4 changes: 2 additions & 2 deletions runtime/moonbase/tests/integration_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1228,7 +1228,7 @@ fn asset_erc20_precompiles_approve() {
let expected_result = Some(Ok(PrecompileOutput {
exit_status: ExitSucceed::Returned,
output: Default::default(),
cost: 19035u64,
cost: 16035u64,
logs: LogsBuilder::new(asset_precompile_address)
.log3(
SELECTOR_LOG_APPROVAL,
Expand Down Expand Up @@ -1261,7 +1261,7 @@ fn asset_erc20_precompiles_approve() {
let expected_result = Some(Ok(PrecompileOutput {
exit_status: ExitSucceed::Returned,
output: Default::default(),
cost: 36042u64,
cost: 31042u64,
logs: LogsBuilder::new(asset_precompile_address)
.log3(
SELECTOR_LOG_TRANSFER,
Expand Down
10,269 changes: 10,269 additions & 0 deletions tests/contracts/compiled/ERC20Instance.json

Large diffs are not rendered by default.

133 changes: 133 additions & 0 deletions tests/contracts/sources.ts
Original file line number Diff line number Diff line change
Expand Up @@ -839,4 +839,137 @@ export const contractSources: { [key: string]: string } = {
);
}
}`,
ERC20Instance: `
// SPDX-License-Identifier: GPL-3.0-only
pragma solidity ^0.8.0;

/**
* @title ERC20 interface
* @dev see https://github.com/ethereum/EIPs/issues/20
* @dev copied from https://github.com/OpenZeppelin/openzeppelin-contracts
*/
interface IERC20 {
/**
* @dev Total number of tokens in existence
* Selector: 18160ddd
*/
function totalSupply() external view returns (uint256);

/**
* @dev Gets the balance of the specified address.
* Selector: 70a08231
* @param who The address to query the balance of.
* @return An uint256 representing the amount owned by the passed address.
*/
function balanceOf(address who) external view returns (uint256);

/**
* @dev Function to check the amount of tokens that an owner allowed to a spender.
* Selector: dd62ed3e
* @param owner address The address which owns the funds.
* @param spender address The address which will spend the funds.
* @return A uint256 specifying the amount of tokens still available for the spender.
*/
function allowance(address owner, address spender)
external view returns (uint256);

/**
* @dev Transfer token for a specified address
* Selector: a9059cbb
* @param to The address to transfer to.
* @param value The amount to be transferred.
*/
function transfer(address to, uint256 value) external returns (bool);

/**
* @dev Approve the passed address to spend the specified amount of tokens on behalf
* of msg.sender.
* Beware that changing an allowance with this method brings the risk that someone may
* use both the old
* and the new allowance by unfortunate transaction ordering. One possible solution to
* mitigate this race condition is to first reduce the spender's allowance to 0 and set
* the desired value afterwards:
* https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729
* Selector: 095ea7b3
* @param spender The address which will spend the funds.
* @param value The amount of tokens to be spent.
*/
function approve(address spender, uint256 value)
external returns (bool);

/**
* @dev Transfer tokens from one address to another
* Selector: 23b872dd
* @param from address The address which you want to send tokens from
* @param to address The address which you want to transfer to
* @param value uint256 the amount of tokens to be transferred
*/
function transferFrom(address from, address to, uint256 value)
external returns (bool);

/**
* @dev Event emited when a transfer has been performed.
* Selector: ddf252ad1be2c89b69c2b068fc378daa952ba7f163c4a11628f55a4df523b3ef
* @param from address The address sending the tokens
* @param to address The address receiving the tokens.
* @param value uint256 The amount of tokens transfered.
*/
event Transfer(
address indexed from,
address indexed to,
uint256 value
);

/**
* @dev Event emited when an approval has been registered.
* Selector: 8c5be1e5ebec7d5bd14f71427d1e84f3dd0314c0f7b2291e5b200ac8c7c3b925
* @param owner address Owner of the tokens.
* @param spender address Allowed spender.
* @param value uint256 Amount of tokens approved.
*/
event Approval(
address indexed owner,
address indexed spender,
uint256 value
);
}

contract ERC20Instance is IERC20 {

/// The ierc20 at the known pre-compile address.
IERC20 public erc20 = IERC20(0x0000000000000000000000000000000000000802);

function totalSupply() override external view returns (uint256){
// We nominate our target collator with all the tokens provided
return erc20.totalSupply();
}

function balanceOf(address who) override external view returns (uint256){
// We nominate our target collator with all the tokens provided
return erc20.balanceOf(who);
}

function allowance(
address owner,
address spender
) override external view returns (uint256){
return erc20.allowance(owner, spender);
}

function transfer(address to, uint256 value) override external returns (bool) {
return erc20.transfer(to, value);
}

function approve(address spender, uint256 value) override external returns (bool) {
return erc20.transfer(spender, value);
}

function transferFrom(
address from,
address to,
int256 value
) override external returns (bool) {
return erc20.transferFrom(from, to, value);
}
}`,
};
Loading