Rosetta: Handle single GasCoin transfers #20592

nonvis · 2024-12-11T05:45:04Z

Description

Describe the changes or additions included in this PR.

Test plan

How did you test the new or updated feature?

Release notes

Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.

For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.

vercel · 2024-12-11T05:45:08Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
sui-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 3, 2025 6:08am

3 Skipped Deployments

Name	Status	Preview	Updated (UTC)
multisig-toolkit	⬜️ Ignored (Inspect)	Visit Preview	Jan 3, 2025 6:08am
sui-kiosk	⬜️ Ignored (Inspect)	Visit Preview	Jan 3, 2025 6:08am
sui-typescript-docs	⬜️ Ignored (Inspect)	Visit Preview	Jan 3, 2025 6:08am

nikos-terzo · 2024-12-11T10:40:12Z

crates/sui-rosetta/src/operations.rs

+                for command in commands {
+                    match command {
+                        SuiCommand::TransferObjects(objs, _) => {
+                            if objs.len() == 1 {


Could we instead have this feature handle well if any of the transferred objects is SuiArgument::GasCoin, instead of only when the only object transferred is a gas-coin?

This ofc would need changes below as well. What I am thinking of is checking whether a Gas transfer exists. If it does and it can be matched with balance-changes between gas-coin-owner and recipient, then we replace the two with the appropriate PaySui and correct Gas operations.

Handles if any of the transferred objects is GasCoin

nikos-terzo · 2024-12-11T13:28:04Z

crates/sui-rosetta/src/operations.rs

+                            Some(amount) => amount.value,
+                            None => 0,
+                        };
+                        if account.address == sender {


As @patrickkuo pointed out, we shouldn't use sender but previous gas-coin owner.

nikos-terzo · 2024-12-11T13:52:40Z

crates/sui-rosetta/src/operations.rs

+                        if account.address == sender {
+                            // sender's balance needs to be adjusted for gas
+                            amount -= gas_used;
+                        } else {


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

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

patrickkuo · 2024-12-11T13:16:15Z

crates/sui-rosetta/src/operations.rs

+        let mut is_single_gascoin_transfer = false;
+        match tx {
+            SuiTransactionBlockKind::ProgrammableTransaction(pt) => {
+                let SuiProgrammableTransactionBlock {
+                    inputs: _,
+                    commands,
+                } = &pt;
+                for command in commands {
+                    match command {
+                        SuiCommand::TransferObjects(objs, _) => {
+                            if objs.len() == 1 {
+                                match objs[0] {
+                                    SuiArgument::GasCoin => {
+                                        is_single_gascoin_transfer = true;
+                                    }
+                                    _ => {}
+                                }
+                            }
+                        }
+                        _ => {}
+                    }
+                }
+            }
+            _ => {}
+        }


lets extract this to a function to make it more readable.

I've extracted it to a function.

nikos-terzo · 2024-12-12T10:12:06Z

crates/sui-rosetta/src/operations.rs

+                    inputs: _,
+                    commands,
+                } = &pt;
+                for command in commands {


In case we find a command with gas-coin we can stop iterating.
Maybe something like commands.into_iter().find(|c| { ... }) would be more suitable.

…snfers

nikos-terzo

Argument::GasCoin can be have any index in the transfer-args input. We need to catch these cases as well.
We need to make sure that we do not double-count the gas used in case more than one SuiPay or SuiBalanceChange refer to the gas sender-recipient pair.

nikos-terzo · 2024-12-16T09:20:54Z

crates/sui-rosetta/src/operations.rs

+                    .into_iter()
+                    .find(|command| match command {
+                        SuiCommand::TransferObjects(objs, _) => {
+                            objs.len() > 0 && objs[0] == SuiArgument::GasCoin


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.

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

nikos-terzo · 2024-12-16T11:46:53Z

crates/sui-rosetta/src/operations.rs

+                        // who paid for the txn (this is the format Rosetta wants to process)
+                        operations.push(Operation::gas(prev_gas_owner, gas_used))
+                    }
+                    OperationType::SuiBalanceChange => {


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

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.

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:

SUI is unlocked from somewhere (eg. a Liquidity Pool),

merged with Gas-coin

gas is sent to the recipient.

nikos-terzo · 2024-12-16T11:49:07Z

crates/sui-rosetta/src/operations.rs

+            tx.data.transaction().clone(),
+            tx.data.gas_data().owner,
+            gas_owner,
+            gas_used,
        );



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?

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.

nikos-terzo · 2024-12-16T11:55:07Z

crates/sui-rosetta/tests/end_to_end_tests.rs

+            }
+        });
+    assert_eq!(balance, 0);
+}


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.

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

…d in to objectTransfers

…perations are processed

…nt by gas

nikos-terzo · 2024-12-18T13:36:09Z

crates/sui-rosetta/src/operations.rs

+                            }
+                        });
+                    }
+                    _ => {}


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

Done. Returns an Error

nikos-terzo · 2024-12-18T13:37:18Z

crates/sui-rosetta/src/operations.rs

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


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?

Done. I made it return an Error so that it won't be secretly removed from the operations list.

nikos-terzo · 2024-12-18T13:41:38Z

crates/sui-rosetta/src/operations.rs

+                    _ => {}
+                }
+            });
+            // sanity check to make sure all the operations have been processed


I don't see any way that this iterator is not consumed. By sanity check I meant checking that tx-balance-changes much the ones the operations refer to.

Removed the sanity check

nikos-terzo · 2024-12-18T13:49:19Z

crates/sui-rosetta/src/operations.rs

+    ) -> Result<Vec<Operation>, anyhow::Error> {
+        let mut gascoin_transfer_operations = vec![];
+        if Self::is_gascoin_transfer(tx) {
+            coin_change_operations.into_iter().for_each(|operation| {


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

Check whether is_gascoin_transfer.

Check whether SUI balance-changes (SuiBalanceChange with SUI) exist between sender and recipient of gas-object, with a matching amount bigger than gas-used.

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.

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

nikos-terzo · 2024-12-18T14:11:39Z

crates/sui-rosetta/src/operations.rs

+                        // who paid for the txn (this is the format Rosetta wants to process)
+                        operations.push(Operation::gas(prev_gas_owner, gas_used))
+                    }
+                    OperationType::SuiBalanceChange => {


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:

SUI is unlocked from somewhere (eg. a Liquidity Pool),

merged with Gas-coin

gas is sent to the recipient.

…uietly discarding operations

[Rosetta] Process a case where single GasCoin is transferred

ad7295e

nonvis temporarily deployed to sui-typescript-aws-kms-test-env December 11, 2024 05:45 — with GitHub Actions Inactive

nonvis requested review from nikos-terzo, geomel and devan-ko December 11, 2024 05:45

[Rosetta] Add e2e test for transferring single GasCoin

8120d1a

nonvis temporarily deployed to sui-typescript-aws-kms-test-env December 11, 2024 12:08 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs December 11, 2024 12:09 View deployment

nikos-terzo requested a review from patrickkuo December 11, 2024 13:11

nikos-terzo requested changes Dec 11, 2024

View reviewed changes

patrickkuo reviewed Dec 11, 2024

View reviewed changes

nikos-terzo reviewed Dec 12, 2024

View reviewed changes

[Rosetta] Support the case when a sponsor pays for single GasCoin tra…

0b6a826

…snfers

nonvis temporarily deployed to sui-typescript-aws-kms-test-env December 16, 2024 06:08 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs December 16, 2024 06:10 View deployment

nonvis requested review from nikos-terzo and patrickkuo December 16, 2024 06:12

nikos-terzo requested changes Dec 16, 2024

View reviewed changes

[Rosetta] Handle the case when GasCoin is not the only argument passe…

ce31fb1

…d in to objectTransfers

nonvis temporarily deployed to sui-typescript-aws-kms-test-env December 17, 2024 02:37 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs December 17, 2024 02:38 View deployment

[Rosetta] Add more comments and a sanity check to make sure all the o…

b5fe666

…perations are processed

nonvis temporarily deployed to sui-typescript-aws-kms-test-env December 18, 2024 08:53 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs December 18, 2024 08:55 View deployment

nonvis requested a review from nikos-terzo December 18, 2024 09:29

Test parsing of tx with custom coin balance changes

69e3c59

nikos-terzo temporarily deployed to sui-typescript-aws-kms-test-env December 18, 2024 10:57 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs December 18, 2024 10:58 View deployment

[Rosetta] fix: Make sure to check currency is SUI when adjusting amou…

f413381

…nt by gas

nonvis temporarily deployed to sui-typescript-aws-kms-test-env December 18, 2024 11:42 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs December 18, 2024 11:43 View deployment

nikos-terzo requested changes Dec 18, 2024

View reviewed changes

[Rosetta] refactor: Return an Error to stop the parsing rather than q…

7903670

…uietly discarding operations

nonvis temporarily deployed to sui-typescript-aws-kms-test-env January 3, 2025 06:07 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs January 3, 2025 06:08 View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rosetta: Handle single GasCoin transfers #20592

Rosetta: Handle single GasCoin transfers #20592

nonvis commented Dec 11, 2024

vercel bot commented Dec 11, 2024 •

edited

Loading

nikos-terzo Dec 11, 2024

nikos-terzo Dec 11, 2024 •

edited

Loading

nonvis Dec 16, 2024

nikos-terzo Dec 11, 2024

nonvis Dec 16, 2024

nikos-terzo Dec 11, 2024

nikos-terzo Dec 11, 2024 •

edited

Loading

nonvis Dec 16, 2024

patrickkuo Dec 11, 2024

nonvis Dec 16, 2024

nikos-terzo Dec 12, 2024

nonvis Dec 16, 2024

nikos-terzo left a comment

nikos-terzo Dec 16, 2024

nonvis Dec 17, 2024

nikos-terzo Dec 16, 2024

nonvis Dec 17, 2024

nikos-terzo Dec 18, 2024 •

edited

Loading

nikos-terzo Dec 16, 2024

nonvis Dec 17, 2024

nikos-terzo Dec 16, 2024

nikos-terzo Dec 16, 2024

nikos-terzo Dec 18, 2024

nonvis Jan 3, 2025

nikos-terzo Dec 18, 2024

nonvis Jan 3, 2025

nikos-terzo Dec 18, 2024

nonvis Jan 3, 2025

nikos-terzo Dec 18, 2024 •

edited

Loading

nikos-terzo Dec 18, 2024 •

edited

Loading

nikos-terzo Dec 18, 2024 •

edited

Loading

Rosetta: Handle single GasCoin transfers #20592

Are you sure you want to change the base?

Rosetta: Handle single GasCoin transfers #20592

Conversation

nonvis commented Dec 11, 2024

Description

Test plan

Release notes

vercel bot commented Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

nikos-terzo Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikos-terzo Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikos-terzo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikos-terzo Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikos-terzo Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

nikos-terzo Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

nikos-terzo Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

vercel bot commented Dec 11, 2024 •

edited

Loading

nikos-terzo Dec 11, 2024 •

edited

Loading

nikos-terzo Dec 11, 2024 •

edited

Loading

nikos-terzo Dec 18, 2024 •

edited

Loading

nikos-terzo Dec 18, 2024 •

edited

Loading

nikos-terzo Dec 18, 2024 •

edited

Loading

nikos-terzo Dec 18, 2024 •

edited

Loading