Skip to content
This repository was archived by the owner on Oct 1, 2024. It is now read-only.

Add updated transaction lifecycle (send + confirm) logic - #75

Open
jshiohaha wants to merge 2 commits into
metaDAOproject:masterfrom
jshiohaha:feature/add-new-transaction-lifecycle-logic
Open

Add updated transaction lifecycle (send + confirm) logic#75
jshiohaha wants to merge 2 commits into
metaDAOproject:masterfrom
jshiohaha:feature/add-new-transaction-lifecycle-logic

Conversation

@jshiohaha

@jshiohaha jshiohaha commented Apr 13, 2024

Copy link
Copy Markdown
Contributor

🚧 Logic should be ready for review, pending PR description & info

Minimum required code to invoke send and confirm a transaction (with defaults applied) 🙂 Otherwise, caller can pretty specifically define a variety of behaviors in both send and confirm logic.

// assume connection and tx are previously defined
const signature = await sendAndConfirmTransaction({
  signedTransaction: tx,
  connection,
});

Associated with the following FaaS task: Futarchy


// note: if we want to try and get more information about a transaction failure, we could make a simulate transaction request here

// question: is there any additional processing we can do to give back more information to the caller?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Indeed, depends on that triggers this. However if we're subscribed we may be able to subscribe logs for this txn and parse the logs, that's the only way, unless there's something I've missed before.

Also FYI, not all RPCs process log messages, so that's a fun nuance ;)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about the subscription case — I see there's an onLogs web socket, but it looks like it's for a specific public key? BUT, if we only care about trying to parse logs on failed transactions, we could get those (kinda for free) if we want to simulate the failed transaction anyways 🙂

I didn't know they might be there depending on the RPC, so thanks for pointing out! 🙌

Comment thread lib/transactions/sendAndConfirm.ts Outdated
});
}

// note: if we want to try and get more information about a transaction failure, we could make a simulate transaction request here

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does simulation afford us an estimate on compute? I want to get better with estimating that. Just a question.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It looks like the simulateTransaction response should contain units consumed (per RPC docs). I also manually checked on a locally running instance of the solana-test-validator (v1.16.20) and was able to see the unitsConsumed field in the output:

Transaction 5wVxmmGRqaeYDDbp1EeLzZR6HZ5ZcCbnmPWxxWBFDeARDYRinKa42ArkUeS97F9Tycq1vRxwP1Gg8pgQQRhb7GiH simulation result:  {
  accounts: null,
  err: { InstructionError: [ 3, 'IllegalOwner' ] },
  logs: [
    'Program 11111111111111111111111111111111 invoke [1]',
    'Program 11111111111111111111111111111111 success',
    'Program 11111111111111111111111111111111 invoke [1]',
    'Program 11111111111111111111111111111111 success',
    'Program 11111111111111111111111111111111 invoke [1]',
    'Program 11111111111111111111111111111111 success',
    'Program opnb2LAfJYbRMAHHvqjCwQxanZn7ReEHp1k81EohpZb invoke [1]',
    'Program log: Instruction: CreateMarket',
    'Program 11111111111111111111111111111111 invoke [2]',
    'Program 11111111111111111111111111111111 success',
    'Program ATokenGPvbdGVxr1b2hvZbsiqW5xWH25efTNsLJA8knL invoke [2]',
    'Program log: Create',
    'Program TokenkegQfeZyiNwAJbNbGKPFXCWuBvf9Ss623VQ5DA invoke [3]',
    'Program log: Instruction: GetAccountDataSize',
    'Program TokenkegQfeZyiNwAJbNbGKPFXCWuBvf9Ss623VQ5DA consumed 1622 of 971299 compute units',
    'Program return: TokenkegQfeZyiNwAJbNbGKPFXCWuBvf9Ss623VQ5DA pQAAAAAAAAA=',
    'Program TokenkegQfeZyiNwAJbNbGKPFXCWuBvf9Ss623VQ5DA success',
    'Program 11111111111111111111111111111111 invoke [3]',
    'Program 11111111111111111111111111111111 success',
    'Program log: Initialize the associated token account',
    'Program TokenkegQfeZyiNwAJbNbGKPFXCWuBvf9Ss623VQ5DA invoke [3]',
    'Program log: Instruction: InitializeImmutableOwner',
    'Program log: Please upgrade to SPL Token 2022 for immutable owner support',
    'Program TokenkegQfeZyiNwAJbNbGKPFXCWuBvf9Ss623VQ5DA consumed 1405 of 964659 compute units',
    'Program TokenkegQfeZyiNwAJbNbGKPFXCWuBvf9Ss623VQ5DA success',
    'Program TokenkegQfeZyiNwAJbNbGKPFXCWuBvf9Ss623VQ5DA invoke [3]',
    'Program log: Instruction: InitializeAccount3',
    'Program TokenkegQfeZyiNwAJbNbGKPFXCWuBvf9Ss623VQ5DA consumed 4241 of 960775 compute units',
    'Program TokenkegQfeZyiNwAJbNbGKPFXCWuBvf9Ss623VQ5DA success',
    'Program ATokenGPvbdGVxr1b2hvZbsiqW5xWH25efTNsLJA8knL consumed 20544 of 976774 compute units',
    'Program ATokenGPvbdGVxr1b2hvZbsiqW5xWH25efTNsLJA8knL success',
    'Program ATokenGPvbdGVxr1b2hvZbsiqW5xWH25efTNsLJA8knL invoke [2]',
    'Program log: Create',
    'Program ATokenGPvbdGVxr1b2hvZbsiqW5xWH25efTNsLJA8knL consumed 3558 of 947720 compute units',
    'Program ATokenGPvbdGVxr1b2hvZbsiqW5xWH25efTNsLJA8knL failed: Provided owner is not allowed',
    'Program opnb2LAfJYbRMAHHvqjCwQxanZn7ReEHp1k81EohpZb consumed 55388 of 999550 compute units',
    'Program opnb2LAfJYbRMAHHvqjCwQxanZn7ReEHp1k81EohpZb failed: Provided owner is not allowed'
  ],
  returnData: null,
  unitsConsumed: 450
}

I already have a working function to plug in. I didn't push in the first round since it wasn't directly called, but I can add it and see what you think. So, it should definitely be possible to detect (+ log) instances in where transactions fail due to improper compute unit limits.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok with the newest commit, if a transaction fails, we will attempt to simulate it. If the simulation is successful, we emit an event with the simulation result that contains the unitsConsumed. We also have the opportunity to try and parse logs if they exist.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I now realize you might have meant "can we simulate and grab units consumed for every transaction", so apologies if I misunderstood. But, it looks we can incorporate if we want.

@R-K-H R-K-H left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks great to me. It's a good first pass and builds a TON of stuff all of which follows a great configurable flow. Gives flexibility and affords A TON of feedback to be surfaced to the caller. There was only one comment about how to get more information I provided, but likely to be used later / in another way.

@jshiohaha
jshiohaha marked this pull request as ready for review April 23, 2024 18:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants