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
92 changes: 89 additions & 3 deletions crates/sui-rosetta/src/operations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -558,6 +558,83 @@ impl Operations {
};
balance_change.chain(gas)
}

fn is_single_gascoin_transfer(tx: SuiTransactionBlockKind) -> bool {
match tx {
SuiTransactionBlockKind::ProgrammableTransaction(pt) => {
let SuiProgrammableTransactionBlock {
inputs: _,
commands,
} = &pt;
return commands
.into_iter()
.find(|command| match command {
SuiCommand::TransferObjects(objs, _) => {
objs.len() > 0 && objs[0] == SuiArgument::GasCoin
Copy link
Contributor

Choose a reason for hiding this comment

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

This would fail if the GasCoin is not the first object transferred.
Something like objs.find(|&obj| obj == SuiArgument::GasCoin).is_some() objs.into_iter().any(|&obj| obj == SuiArgument::GasCoin) would succeed in case any of the objects transferred is the GasCoin.
I guess as we should identify such transfers, we can also rename the function to is_gascoin_transfer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a search for any place for GasCoin and renamed the function to is_gascoin_transfer

}
_ => false,
})
.is_some();
}
_ => {}
}
false
}

fn process_single_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,
) -> Vec<Operation> {
let mut operations = vec![];
if Self::is_single_gascoin_transfer(tx) {
coin_change_operations.into_iter().for_each(|operation| {
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.

I am wondering whether it would be cleaner, to replace this mapping of balance-changes to a more explicit approach:

  1. Check whether is_gascoin_transfer.
  2. Check whether SUI balance-changes (SuiBalanceChange with SUI) exist between sender and recipient of gas-object, with a matching amount bigger than gas-used.
  3. If both of the above are true, edit these 3 operations:
    • recipient Gas
    • sender balance-change of SUI -> PaySui
    • recipient balance-change of SUI -> PaySui,
      otherwise if 1 is true but 2 is not*, create the missing PaySui operation. Maybe create two new SuiBalanceChange operations (other balance-changes might exist for sender/recipient, for different reasons).

*A little far-fetched, but the sender could have received the exact amount of the gas-coin (no-balance-change), or the sender could have received a larger balance than the gas-coin, making it a PaySui which increases the sender's balance. This shouldn't happen.
Also due to the nature of ptbs, there might be the case that even the recipient could receive more balance than the one which was transferred by the gas-coin, for other reasons.

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.

@patrickkuo , @nonvis wdyt? I believe explicitly editing the operations in question, instead of mapping all the operations is a better approach.

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)
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.

operation.account.map(|account| {
let mut amount = match operation.amount {
Some(amount) => amount,
None => return,
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we ignore an Operation. The operation will be removed from the operations list. Is this case unreachable? If so, can we return an error with the tx-digest?

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. I made it return an Error so that it won't be secretly removed from the operations list.

};
let mut is_convert_to_pay_sui = false;
if account.address == prev_gas_owner {
// 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 {
// 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 {
operations.push(Operation::pay_sui(
operation.status,
account.address,
amount.value,
));
} else {
operations.push(Operation::balance_change(
operation.status,
account.address,
amount.value,
amount.currency,
));
}
});
}
_ => {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Theoretically this should be unreachable, correct? If so, can we return an Error here?

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. Returns an Error

}
});
}
operations
}
}

impl Operations {
Expand Down Expand Up @@ -592,7 +669,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 +739,26 @@ 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(),
);

let single_gascoin_transfer = Self::process_single_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(single_gascoin_transfer)
.chain(staking_balance)
.collect();

Expand Down
133 changes: 129 additions & 4 deletions crates/sui-rosetta/tests/end_to_end_tests.rs
Original file line number Diff line number Diff line change
@@ -1,23 +1,34 @@
// Copyright (c) Mysten Labs, Inc.
// SPDX-License-Identifier: Apache-2.0

use anyhow::anyhow;
use rand::rngs::OsRng;
use rand::seq::IteratorRandom;
use rosetta_client::start_rosetta_test_server;
use serde_json::json;
use shared_crypto::intent::Intent;
use std::num::NonZeroUsize;
use std::time::Duration;

use rosetta_client::start_rosetta_test_server;
use sui_json_rpc_types::SuiTransactionBlockResponseOptions;
use sui_json_rpc_types::{
SuiObjectDataOptions, SuiObjectResponseQuery, SuiTransactionBlockResponseOptions,
};
use sui_keys::keystore::AccountKeystore;
use sui_rosetta::operations::Operations;
use sui_rosetta::types::Currencies;
use sui_rosetta::types::{
AccountBalanceRequest, AccountBalanceResponse, AccountIdentifier, Currency, NetworkIdentifier,
SubAccount, SubAccountType, SuiEnv,
};
use sui_rosetta::types::{Currencies, OperationType};
use sui_rosetta::CoinMetadataCache;
use sui_sdk::rpc_types::{SuiExecutionStatus, SuiTransactionBlockEffectsAPI};
use sui_sdk::SuiClient;
use sui_swarm_config::genesis_config::{DEFAULT_GAS_AMOUNT, DEFAULT_NUMBER_OF_OBJECT_PER_ACCOUNT};
use sui_types::base_types::{ObjectID, ObjectRef, SuiAddress};
use sui_types::programmable_transaction_builder::ProgrammableTransactionBuilder;
use sui_types::quorum_driver_types::ExecuteTransactionRequestType;
use sui_types::transaction::{
Argument, InputObjectKind, Transaction, TransactionData, TEST_ONLY_GAS_UNIT_FOR_TRANSFER,
};
use sui_types::utils::to_sender_signed_transaction;
use test_cluster::TestClusterBuilder;

Expand Down Expand Up @@ -501,3 +512,117 @@ async fn test_pay_sui_multiple_times() {
);
}
}

async fn get_random_sui(
client: &SuiClient,
sender: SuiAddress,
except: Vec<ObjectID>,
) -> ObjectRef {
let coins = client
.read_api()
.get_owned_objects(
sender,
Some(SuiObjectResponseQuery::new_with_options(
SuiObjectDataOptions::new()
.with_type()
.with_owner()
.with_previous_transaction(),
)),
/* cursor */ None,
/* limit */ None,
)
.await
.unwrap()
.data;

let coin_resp = coins
.iter()
.filter(|object| {
let obj = object.object().unwrap();
obj.is_gas_coin() && !except.contains(&obj.object_id)
})
.choose(&mut OsRng)
.unwrap();

let coin = coin_resp.object().unwrap();
(coin.object_id, coin.version, coin.digest)
}
#[tokio::test]
async fn test_transfer_single_gas_coin() {
let test_cluster = TestClusterBuilder::new().build().await;
let sender = test_cluster.get_address_0();
let recipient = test_cluster.get_address_1();
let client = test_cluster.wallet.get_client().await.unwrap();
let keystore = &test_cluster.wallet.config.keystore;

let pt = {
let mut builder = ProgrammableTransactionBuilder::new();
builder.transfer_arg(recipient, Argument::GasCoin);
builder.finish()
};

let input_objects = pt
.input_objects()
.unwrap_or_default()
.iter()
.flat_map(|obj| {
if let InputObjectKind::ImmOrOwnedMoveObject((id, ..)) = obj {
Some(*id)
} else {
None
}
})
.collect::<Vec<_>>();
let gas = vec![get_random_sui(&client, sender, input_objects).await];
let gas_price = client
.governance_api()
.get_reference_gas_price()
.await
.unwrap();

let data = TransactionData::new_programmable(
sender,
gas,
pt,
TEST_ONLY_GAS_UNIT_FOR_TRANSFER * gas_price,
gas_price,
);

let signature = keystore
.sign_secure(&sender, &data, Intent::sui_transaction())
.unwrap();

let response = client
.quorum_driver_api()
.execute_transaction_block(
Transaction::from_data(data.clone(), vec![signature]),
SuiTransactionBlockResponseOptions::new()
.with_effects()
.with_object_changes()
.with_balance_changes()
.with_input(),
Some(ExecuteTransactionRequestType::WaitForLocalExecution),
)
.await
.map_err(|e| anyhow!("TX execution failed for {data:#?}, error : {e}"))
.unwrap();

let coin_cache = CoinMetadataCache::new(client, NonZeroUsize::new(2).unwrap());
let operations = Operations::try_from_response(response, &coin_cache)
.await
.unwrap();
println!("operations: {operations:#?}");

let mut balance = 0;
operations
.into_iter()
.for_each(|op| {
if op.type_ == OperationType::Gas {
assert_eq!(op.account.unwrap().address, sender);
}
if op.type_ == OperationType::PaySui {
balance += op.amount.unwrap().value;
}
});
assert_eq!(balance, 0);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Optimally, a more complex transaction involving gas-coin transfer should be tested too. Something involving a balance-change from eg. a DEX tx along with a gas-coin transfer. I know this is an extreme edge-case, but if not every, almost every tx on-chain will actually be passed through this endpoint.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can copy an existing DEX tx from chain and try manually adding a gas-coin-transfer to it?

Loading