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

unstable_rpc: Add transactionBroadcast and transactionStop #1497

Merged
merged 9 commits into from
Mar 26, 2024

Conversation

lexnv
Copy link
Collaborator

@lexnv lexnv commented Mar 21, 2024

This PR adds the transaction_unstable_broadcast and transaction_unstable_stop APIs.

While at it, added a test to check that the broadcasted transaction gets into a finalized block.

@lexnv lexnv self-assigned this Mar 21, 2024
@lexnv lexnv requested a review from a team as a code owner March 21, 2024 16:03
Comment on lines 355 to 363
let block_extrinsics = extrinsics
.iter()
.map(|res| res.unwrap())
.collect::<Vec<_>>();

// All blocks must contain the timestamp, we expect the next extrinsics to be our own.
let Some(tx) = block_extrinsics.get(1) else {
continue;
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: this is all good, but if we wanted we could do something like this to avoid relying on any sort of ordering etc:

let tx_hash = SubstrateConfig::Hashing::hash_of(tx_bytes);

// ...

let Some(ext) = extrinsics
    .iter()
    .find(|e| SubstrateConfig::Hashing::hash_of(e.bytes()) == tx_hash)
    .next() else { continue };

let ext = ext.as_extrinsic::<node_runtime::balances::calls::types::TransferAllowDeath>()
    .unwrap()
    .unwrap();

// ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is just a small inconsistency with this:

  • create_signed_offline returns the extrinsic bytes including the prefix of compact encoding length
  • ExtrinsicDetails will strip the compact prefix:
    let bytes: Arc<[u8]> = strip_compact_prefix(extrinsic_bytes)?.1.into();

I've adjusted the code to reflect this, not sure if we should do anything else to align for ex with polkadot-api 🤔

///
/// Returns an operation ID that can be used to stop the broadcasting process.
/// Returns `None` if the server cannot handle the request at the moment.
pub async fn transaction_unstable_broadcast(&self, tx: &[u8]) -> Result<Option<String>, Error> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Has the PR to rename to v1 merged in Substrate already? :)

Copy link
Member

@niklasad1 niklasad1 Mar 21, 2024

Choose a reason for hiding this comment

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

Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

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

A couple of minor things but yay for more RPCs :)

When this merges we can also move our TX submitting to use this method and thus be "reconnect" safe at some point which will be nice (though much more complicated, because aside from searching blocks for the TX we need to periodically validate it to know whether to stop checking for it etc).

lexnv and others added 4 commits March 22, 2024 11:44
@lexnv lexnv merged commit 92c1ba7 into master Mar 26, 2024
13 checks passed
@lexnv lexnv deleted the lenxv/validate-tx-broadcast branch March 26, 2024 15:14
@jsdw jsdw mentioned this pull request May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants