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

Rosetta: Handle single GasCoin transfers #20592

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
102 changes: 99 additions & 3 deletions crates/sui-rosetta/src/operations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -558,6 +558,91 @@ impl Operations {
};
balance_change.chain(gas)
}

/// Checks to see if transferObjects is used on GasCoin
fn is_gascoin_transfer(tx: SuiTransactionBlockKind) -> bool {
match tx {
SuiTransactionBlockKind::ProgrammableTransaction(pt) => {
let SuiProgrammableTransactionBlock {
inputs: _,
commands,
} = &pt;
return commands.iter().any(|command| match command {
SuiCommand::TransferObjects(objs, _) => {
objs.iter().any(|&obj| obj == SuiArgument::GasCoin)
}
_ => false,
});
}
_ => {}
}
false
}

/// If GasCoin is transferred as a part of transferObjects, operations need to be
/// updated such that:
/// 1) gas owner needs to be assigned back to the previous owner
/// 2) SuiBalanceChange type needs to be converted to PaySui for
/// previous and new gas owners and their balances need to be adjusted for the gas
fn process_gascoin_transfer(
coin_change_operations: &mut impl Iterator<Item = crate::operations::Operation>,
tx: SuiTransactionBlockKind,
prev_gas_owner: SuiAddress,
new_gas_owner: SuiAddress,
gas_used: i128,
) -> Result<Vec<Operation>, anyhow::Error> {
let mut gascoin_transfer_operations = vec![];
if Self::is_gascoin_transfer(tx) {
for operation in coin_change_operations.into_iter() {
match operation.type_ {
OperationType::Gas => {
// change gas account back to the previous owner as it is the one
// who paid for the txn (this is the format Rosetta wants to process)
gascoin_transfer_operations.push(Operation::gas(prev_gas_owner, gas_used))
}
OperationType::SuiBalanceChange => {
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if multiple balance changes exist there for sender or recipient? I think this block might double-count gas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

balance_changes are derived from effects field and even though there were multiple transfers for sender/recipient, there is one entry per address so I think there is no risk of double-count here.

Copy link
Contributor

@nikos-terzo nikos-terzo Dec 18, 2024

Choose a reason for hiding this comment

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

Could there be the case though, that the sender also receives an amount equal to the gas-coin transferred, so that there is no balance-change for them? I know that this is far-fetched, but every SUI tx passes through this parsing. In that case this operation mapping would fail, correct? Lastly, the PaySui operations need to have their amounts match. See also above comment about explicit handling versus mapping.

Edit: Also even if the amount is not equal, it would introduce a discrepancy between the PaySui for receiver and the PaySui for sender operations.
Example ptb:

  1. SUI is unlocked from somewhere (eg. a Liquidity Pool),
  2. merged with Gas-coin
  3. gas is sent to the recipient.

let account = operation
.account
.ok_or_else(|| anyhow!("Missing account for a balance-change"))?;
let mut amount = operation
.amount
.ok_or_else(|| anyhow!("Missing amount for a balance-change"))?;
let mut is_convert_to_pay_sui = false;
if account.address == prev_gas_owner && amount.currency == *SUI {
// previous owner's balance needs to be adjusted for gas
amount.value -= gas_used;
is_convert_to_pay_sui = true;
} else if account.address == new_gas_owner && amount.currency == *SUI {
// new owner's balance needs to be adjusted for gas
amount.value += gas_used;
is_convert_to_pay_sui = true;
}
if is_convert_to_pay_sui {
gascoin_transfer_operations.push(Operation::pay_sui(
operation.status,
account.address,
amount.value,
));
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not edit every balance-change. Only the gas-coin sender and the recipient ones.

Copy link
Contributor

@nikos-terzo nikos-terzo Dec 11, 2024

Choose a reason for hiding this comment

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

And we should also make sure that these addition and subtraction each happen only once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

gascoin_transfer_operations.push(Operation::balance_change(
operation.status,
account.address,
amount.value,
amount.currency,
));
}
}
_ => {
return Err(anyhow!(
"Discarding unsupported operation type {:?}",
operation.type_
))
}
}
}
}
Ok(gascoin_transfer_operations)
}
}

impl Operations {
Expand Down Expand Up @@ -592,7 +677,7 @@ impl Operations {
- gas_summary.computation_cost as i128;

let status = Some(effect.into_status().into());
let ops = Operations::try_from_data(tx.data, status)?;
let ops = Operations::try_from_data(tx.data.clone(), status)?;
let ops = ops.into_iter();

// We will need to subtract the operation amounts from the actual balance
Expand Down Expand Up @@ -662,17 +747,28 @@ impl Operations {
}

// Extract coin change operations from balance changes
let coin_change_operations = Self::process_balance_change(
let mut coin_change_operations = Self::process_balance_change(
gas_owner,
gas_used,
balance_changes,
status,
accounted_balances,
accounted_balances.clone(),
);

// Take {gas, previous gas owner, new gas owner} out of coin_change_operations
// and convert BalanceChange to PaySui when GasCoin is transferred
let gascoin_transfer_operations = Self::process_gascoin_transfer(
&mut coin_change_operations,
tx.data.transaction().clone(),
tx.data.gas_data().owner,
gas_owner,
gas_used,
)?;

Copy link
Contributor

Choose a reason for hiding this comment

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

Every transaction that happens on-chain will pass from here. We need to be extra careful that we do not mess up the balance-changes. Does it make sense to add a sanity check that operations balance changes match the tx balance changes after editing the operations with gas-coins transfer?
@patrickkuo , @nonvis wdyt?

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 don't think a sanity check is required. First, the balance-changes is edited only when GasCoin is referenced for transferObjects, which occurs infrequently. And if they do occur, only two types of operations are used: BalanceChange and Gas. process_gascoin_transfer function processes these two types of operations.
So in terms of the number of operations stored before/after process_gascoin_transfer, they would be the same.

let ops: Operations = ops
.into_iter()
.chain(coin_change_operations)
.chain(gascoin_transfer_operations)
.chain(staking_balance)
.collect();

Expand Down
158 changes: 153 additions & 5 deletions crates/sui-rosetta/tests/custom_coins_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,23 +6,36 @@ mod rosetta_client;
#[path = "custom_coins/test_coin_utils.rs"]
mod test_coin_utils;

use serde_json::json;
use std::num::NonZeroUsize;
use std::path::Path;
use std::str::FromStr;

use serde_json::json;

use shared_crypto::intent::Intent;
use sui_json_rpc_types::{
SuiExecutionStatus, SuiTransactionBlockEffectsAPI, SuiTransactionBlockResponseOptions,
ObjectChange, SuiExecutionStatus, SuiTransactionBlockEffectsAPI,
SuiTransactionBlockResponseOptions,
};
use sui_keys::keystore::AccountKeystore;
use sui_rosetta::operations::Operations;
use sui_rosetta::types::{
AccountBalanceRequest, AccountBalanceResponse, AccountIdentifier, Currency, CurrencyMetadata,
NetworkIdentifier, SuiEnv,
AccountBalanceRequest, AccountBalanceResponse, AccountIdentifier, Amount, Currency,
CurrencyMetadata, NetworkIdentifier, SuiEnv,
};
use sui_rosetta::types::{Currencies, OperationType};
use sui_rosetta::CoinMetadataCache;
use sui_rosetta::SUI;
use sui_types::coin::COIN_MODULE_NAME;
use sui_types::programmable_transaction_builder::ProgrammableTransactionBuilder;
use sui_types::quorum_driver_types::ExecuteTransactionRequestType;
use sui_types::transaction::{
Argument, Command, ObjectArg, Transaction, TransactionData, TransactionDataAPI,
};
use sui_types::{Identifier, SUI_FRAMEWORK_PACKAGE_ID};

use test_cluster::TestClusterBuilder;
use test_coin_utils::{init_package, mint};

use crate::rosetta_client::{start_rosetta_test_server, RosettaEndpoint};

#[tokio::test]
Expand Down Expand Up @@ -301,3 +314,138 @@ async fn test_custom_coin_without_symbol() {
}
}
}

#[tokio::test]
async fn test_mint_with_gas_coin_transfer() -> anyhow::Result<()> {
const COIN1_BALANCE: u64 = 100_000_000;
const COIN2_BALANCE: u64 = 200_000_000;
let test_cluster = TestClusterBuilder::new().build().await;
let client = test_cluster.wallet.get_client().await.unwrap();
let keystore = &test_cluster.wallet.config.keystore;

let sender = test_cluster.get_address_0();
let init_ret = init_package(
&client,
keystore,
sender,
Path::new("tests/custom_coins/test_coin"),
)
.await
.unwrap();

let address1 = test_cluster.get_address_1();
let address2 = test_cluster.get_address_2();
let balances_to = vec![(COIN1_BALANCE, address1), (COIN2_BALANCE, address2)];

let treasury_cap_owner = init_ret.owner;

let gas_price = client
.governance_api()
.get_reference_gas_price()
.await
.unwrap();
const LARGE_GAS_BUDGET: u128 = 1_000_000_000;
let mut gas_coins = client
.coin_read_api()
.select_coins(sender, None, LARGE_GAS_BUDGET, vec![])
.await?;
assert!(
gas_coins.len() == 1,
"Expected 1 large gas-coin to satisfy the budget"
);
let gas_coin = gas_coins.pop().unwrap();
let mut ptb = ProgrammableTransactionBuilder::new();

let treasury_cap = ptb.obj(ObjectArg::ImmOrOwnedObject(init_ret.treasury_cap))?;
for (balance, to) in balances_to {
let balance = ptb.pure(balance)?;
let coin = ptb.command(Command::move_call(
SUI_FRAMEWORK_PACKAGE_ID,
Identifier::from(COIN_MODULE_NAME),
Identifier::from_str("mint")?,
vec![init_ret.coin_tag.clone()],
vec![treasury_cap, balance],
));
ptb.transfer_arg(to, coin);
}
ptb.transfer_arg(address1, Argument::GasCoin);
let builder = ptb.finish();

// Sign transaction
let tx_data = TransactionData::new_programmable(
treasury_cap_owner,
vec![gas_coin.object_ref()],
builder,
LARGE_GAS_BUDGET as u64,
gas_price,
);

let sig = keystore.sign_secure(&tx_data.sender(), &tx_data, Intent::sui_transaction())?;

let mint_res = client
.quorum_driver_api()
.execute_transaction_block(
Transaction::from_data(tx_data, vec![sig]),
SuiTransactionBlockResponseOptions::new()
.with_balance_changes()
.with_effects()
.with_input()
.with_object_changes(),
Some(ExecuteTransactionRequestType::WaitForLocalExecution),
)
.await?;
let gas_used = mint_res.effects.as_ref().unwrap().gas_cost_summary();
let mut gas_used = gas_used.net_gas_usage() as i128;
println!("gas_used: {gas_used}");

let coins = mint_res
.object_changes
.as_ref()
.unwrap()
.iter()
.filter_map(|change| {
if let ObjectChange::Created {
object_type, owner, ..
} = change
{
Some((object_type, owner))
} else {
None
}
})
.collect::<Vec<_>>();
let coin1 = coins
.iter()
.find(|coin| coin.1.get_address_owner_address().unwrap() == address1)
.unwrap();
let coin2 = coins
.iter()
.find(|coin| coin.1.get_address_owner_address().unwrap() == address2)
.unwrap();
assert!(coin1.0.to_string().contains("::test_coin::TEST_COIN"));
assert!(coin2.0.to_string().contains("::test_coin::TEST_COIN"));

let coin_cache = CoinMetadataCache::new(client, NonZeroUsize::new(2).unwrap());
let ops = Operations::try_from_response(mint_res, &coin_cache)
.await
.unwrap();
const COIN_BALANCE_CREATED: u64 = COIN1_BALANCE + COIN2_BALANCE;
println!("ops: {}", serde_json::to_string_pretty(&ops).unwrap());
let mut coin_created = 0;
ops.into_iter().for_each(|op| {
if let Some(Amount {
value, currency, ..
}) = op.amount
{
if currency == Currency::default() {
gas_used += value
} else {
coin_created += value
}
}
});
assert!(COIN_BALANCE_CREATED as i128 == coin_created);
assert!(gas_used == 0);

Ok(())
}
Loading
Loading