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

allow ability to pass in feePayer to Provider methods [WIP] #2186

Merged
merged 12 commits into from Dec 5, 2022

Conversation

surfertas
Copy link
Contributor

Allow passing in an optional feePayer param to the send methods of the Provider class. This allows the caller to specify who pays for the transaction, as opposed to hard coding it to the connected wallet.

closes #2180

Reference discord discussion:

https://discord.com/channels/889577356681945098/891724485177245717/1020066171329982605

@vercel
Copy link

vercel bot commented Sep 16, 2022

@surfertas is attempting to deploy a commit to the coral-xyz Team on Vercel.

A member of the Team first needs to authorize it.

@Henry-E
Copy link
Contributor

Henry-E commented Sep 16, 2022

Instead of adding a new argument, could the code instead check if feePayer has already been set in the transaction and if not then it assigns the provider's wallet?

If you could also add something to the changelog in the PR that references this PR that would be helpful too.

@Henry-E
Copy link
Contributor

Henry-E commented Sep 17, 2022

Looks good to me! Will merge it in shortly

@surfertas surfertas changed the title allow ability to pass in feePayer to Provider methods allow ability to pass in feePayer to Provider methods [WIP] Sep 17, 2022
@surfertas
Copy link
Contributor Author

surfertas commented Sep 17, 2022

@Henry-E Was wondering if we should be using a strict equality check for undefined or use a null check to handle both cases?

@Henry-E
Copy link
Contributor

Henry-E commented Sep 17, 2022

In the default transaction it looks like feePayer is actually undefined

    Transaction {
      signatures: [],
      feePayer: undefined,
      instructions: [],
      recentBlockhash: undefined,
      lastValidBlockHeight: undefined,
      nonceInfo: undefined,
      _message: undefined,
      _json: undefined
    }

so you should be checking

if (tx.feePayer === undefined)

You should probably add a client test then to /misc, to send a tx, then load up the processed transaction to confirm that the fee payer was assigned correctly, since it looks like this code probably wouldn't have worked.

@surfertas
Copy link
Contributor Author

Yeah sounds good, technically the null check will capture undefined as well afiak, but the strict equality check for undefined is more concise! Will put that test up.

@armaniferrante
Copy link
Member

Yeah sounds good, technically the null check will capture undefined as well afiak, but the strict equality check for undefined is more concise! Will put that test up.

I'm not sure this is true. We should check for both?

@surfertas
Copy link
Contributor Author

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/null

tx.feePayer == null will handle both null and undefined. Shall we go with that?

@Henry-E
Copy link
Contributor

Henry-E commented Sep 23, 2022

Sure, if tx.feepayer == null works for both then that should be fine. Just to be safe lets add a client test to tests/misc that sends three transactions

  • one transaction with the usual undefined feepayer
  • one with a custom fee payer
  • one transaction with a feepayer manually set to null
    Then check the transaction signature to confirm that all three have the correct fee payer.

(I might be able to add these tests next week if you aren't able to add them)

@surfertas
Copy link
Contributor Author

@Henry-E sorry hate to ask, but if you have time can you throw up thoses tests? Just got backed up with work. Sincere apologies.

@Henry-E
Copy link
Contributor

Henry-E commented Nov 22, 2022

  • It's a pretty useful added feature that fixes some over reaching behaviour of the anchor provider. Namely it allows the transaction creator to add their own feePayer which isn't over written by the anchor provider default wallet.
  • It doesn't have tests which isn't great but the logic seems simple enough and the logical or || should handle the null case nicely when the feePayer is undefined or null (or 0?).

Just going to wait for tests to run again and then merge.

@Henry-E Henry-E merged commit 50724df into coral-xyz:master Dec 5, 2022
Henry-E added a commit to Henry-E/anchor that referenced this pull request Dec 6, 2022
…z#2186)

* allow ability to pass in feePayer to Provider methods

* align comments

* prettier

* check if feePayer set, handle if not

* remove unnecessary spaces in comments

* update changelog

* strict equality check

* use null check

* use logical or

* use logical or

Co-authored-by: Henry-E <henry.elder@adaptcentre.ie>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow for setting a transaction feePayer, instead of enforcing the connected wallet to always be the feePayer.
3 participants