Skip to content
This repository has been archived by the owner on Sep 14, 2023. It is now read-only.

chore: rework finalized/inBlock with BlockHashRune #800

Merged
merged 6 commits into from
Mar 27, 2023
Merged

Conversation

harrysolovay
Copy link
Contributor

Given that we have inBlockHash/finalizedHash and inBlockEvents/finalizedEvents, there's a chance that finalized alone for describing block would be confusing.

@harrysolovay harrysolovay requested a review from tjjfvi as a code owner March 25, 2023 18:03
@tjjfvi
Copy link
Contributor

tjjfvi commented Mar 27, 2023

I think instead we should rename them back to .finalized() and use .finalized().hash instead of .finalizedHash()

@harrysolovay
Copy link
Contributor Author

harrysolovay commented Mar 27, 2023

Is there a path that would better-position devs to avoid an accidental extra network call (using the hash to retrieve the block)? Thoughts on finalized producing a new BlockHashRune, which would have a block method on it? (same for inBlock).

@tjjfvi
Copy link
Contributor

tjjfvi commented Mar 27, 2023

.finalized().hash wouldn't produce an extra network call because runes are lazy

@harrysolovay
Copy link
Contributor Author

Yes, but let's consider cases where devs just want to await finalization.

await chain
  .extrinsic(Balances.transfer({
    value: 12345n,
    dest: billy.address,
  }))
  .signed(signature({ sender: alexa }))
  .sent()
  .finalized()
  .run()

It's easy to forget the .hash.

@deno-deploy deno-deploy bot requested a deployment to Preview March 27, 2023 14:16 Abandoned
@tjjfvi
Copy link
Contributor

tjjfvi commented Mar 27, 2023

Fair enough – then yeah, a BlockHashRune makes sense

@harrysolovay harrysolovay changed the title chore: rename ExtrinsicStatusRune's finalized/inBlock to finalizedBlock/inBlockBlock chore: rework finalized/inBlock with BlockHashRune Mar 27, 2023
tjjfvi
tjjfvi previously approved these changes Mar 27, 2023
@harrysolovay harrysolovay added this pull request to the merge queue Mar 27, 2023
Merged via the queue into main with commit 36db54a Mar 27, 2023
@harrysolovay harrysolovay deleted the two-renames branch March 27, 2023 15:12
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