Skip to content
This repository has been archived by the owner on Jan 22, 2025. It is now read-only.

Support sending VersionedTransaction in tests #22862

Closed
wants to merge 3 commits into from

Conversation

ckamm
Copy link
Contributor

@ckamm ckamm commented Feb 1, 2022

Problem

  • There was no way to create a VersionedTransaction from instructions and address lookup tables.
  • There was no way to send these transactions in a test through banks-client
  • The Bank rejected them because it didn't support receiving transactions with address maps from banks-server

Summary of Changes

  • Fix prepare_entry_batch() in Bank to deal with lookup table addresses.
  • Add process_versioned_transaction_with_commitment_and_context() etc to banks-{interface,client,server} for use in tests.
  • Add v0::Message::new_* functions to conveniently make instances.

Open questions

  • Check verify_versioned_transaction() in banks_server:
    For non-versioned transactions there's a transaction.verify_precompiles() that I haven't transfered over yet. Are program addresses guaranteed to be in the static address list, and not come from address lookup tables?
  • Check the api and names of the v0::Message::new_*() functions.
    I copied liberally from the legacy message but think naming could be better.
  • Also look at pub struct AddressLookupTable in versions/v0/mod.rs.
    I need information about the contents of the lookup tables to be able to go from Instruction to CompiledInstruction. Is there an existing data structure I should use instead?

@jstarry

@mergify mergify bot added the community Community contribution label Feb 1, 2022
@mergify mergify bot requested a review from a team February 1, 2022 11:17
/// Return pubkeys referenced by all instructions, with the ones needing signatures first. If the
/// payer key is provided, it is always placed first in the list of signed keys. Read-only signed
/// accounts are placed last in the set of signed accounts. Read-only unsigned accounts,
/// including program ids, are placed last in the set. No duplicates and order is preserved.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

need to update this comment

signed_keys,
unsigned_keys,
address_table_keys,
address_table_lookups, // TODO: remove empty ones
Copy link
Contributor Author

@ckamm ckamm Feb 1, 2022

Choose a reason for hiding this comment

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

Remove references to address lookup tables that were not needed by any instruction's accounts.

Err(err)
// TODO: Can't verify precompiles without looking up address map accounts...
//} else if let Err(err) = transaction.verify_precompiles(feature_set) {
// Err(err)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check if verify_precompiles is possible because program accounts are guaranteed to be in the static list?

@jstarry jstarry requested review from jstarry and removed request for a team February 2, 2022 02:47
@mergify mergify bot requested a review from a team February 2, 2022 02:48
@stale
Copy link

stale bot commented Mar 2, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Mar 2, 2022
@ckamm
Copy link
Contributor Author

ckamm commented Mar 2, 2022

This is not stale. The PR is still needed to write tests with address lookup tables.

@stale stale bot removed the stale [bot only] Added to stale content; results in auto-close after a week. label Mar 2, 2022
@jstarry
Copy link
Contributor

jstarry commented Mar 17, 2022

@ckamm I refactored your changes so that logic is shared between the legacy and v0 messages: #23729 and I also broke out the v0 message creation into a new PR here: #23728

@ckamm
Copy link
Contributor Author

ckamm commented Mar 17, 2022

@jstarry Great, so this PR can be closed?

@jstarry
Copy link
Contributor

jstarry commented Mar 18, 2022

@jstarry Great, so this PR can be closed?

Yeah feel free, I'll likely borrow your bank api changes still but will do that in another PR

@stale
Copy link

stale bot commented Apr 16, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Apr 16, 2022
@stale
Copy link

stale bot commented Apr 25, 2022

This stale pull request has been automatically closed. Thank you for your contributions.

@stale stale bot closed this Apr 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
community Community contribution stale [bot only] Added to stale content; results in auto-close after a week.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants