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

Will TransactionResponse type always have r,s,v defined? #1181

Closed
mds1 opened this issue Dec 2, 2020 · 3 comments
Closed

Will TransactionResponse type always have r,s,v defined? #1181

mds1 opened this issue Dec 2, 2020 · 3 comments
Labels
documentation Documentation related issue. fixed/complete This Bug is fixed or Enhancement is complete and published.

Comments

@mds1
Copy link

mds1 commented Dec 2, 2020

The Transaction type has r,s,v as optional fields. The TransactionResponse type extends Transaction, but does not make r,s,v required fields.

Is this an oversight, or are there cases when TransactionResponse will not have r,s,v defined? If the latter, what scenarios? Every transaction must have a valid signature so I'd expect r,s,v to be required in this type.

For context, I'm recovering a public key from a transaction hash, so I noticed this when getting the signature from the transaction:

const tx = await ethersProvider.getTransaction(txHash);
const splitSignature: SignatureLike = {
    r: tx.r as string, // without casting here, TypeScript complains since `tx.r` may be `undefined`
    s: tx.s,
    v: tx.v,
  };
  const signature = joinSignature(splitSignature);

On a side note, it seems getTransaction isn't in the documentation. Am I missing it somehow, or is there an alternate way to do the above?

@ricmoo
Copy link
Member

ricmoo commented Dec 2, 2020

You are right about the docs, I’ll add it shortly.

There were cases where r, s and v were not present. I’m trying to remember why though, but it was deliberate.

Possibly Ganache did not include these? Or Etherscan? I’ll have to scratch my brain tonight and try to recall. But I do remember things breaking when they were required properties.

@ricmoo ricmoo added the documentation Documentation related issue. label Dec 2, 2020
@ricmoo ricmoo added on-deck This Enhancement or Bug is currently being worked on. fixed/complete This Bug is fixed or Enhancement is complete and published. and removed on-deck This Enhancement or Bug is currently being worked on. labels Jun 24, 2021
@ricmoo
Copy link
Member

ricmoo commented Jun 24, 2021

The docs were updated a while ago with the getTransaction method. I can't find examples where r, s and v are not populated, but that is a change I don't want to make in v5. In v6 the Transaction object is much stricter, so they won't be optional.

Thanks! :)

@ricmoo ricmoo closed this as completed Jun 24, 2021
@roflganker
Copy link

@ricmoo

There were cases where r, s and v were not present. I’m trying to remember why though, but it was deliberate.

Well I found a matching case 😅
#3926 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Documentation related issue. fixed/complete This Bug is fixed or Enhancement is complete and published.
Projects
None yet
Development

No branches or pull requests

3 participants