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

feat(ownership-provider): support payFee and signTransaction in sendTransaction #272

Merged
merged 12 commits into from
May 3, 2023

Conversation

IronLu233
Copy link
Contributor

@IronLu233 IronLu233 commented Apr 27, 2023

Release Note

The sendTransaction is smarter now, we don't need to call provider.payFee and provider.signTransaction manually, just send it

What Changed

Resolve #219

This PR make it is easier to use FullOwnershipProvider#sendTransaction, now when call sendTransaction, payFee and sign will also be done if needed.

Motivation

Please see #219 for more details.

Change Type

Indicate the type of change your pull request is:

  • documentation
  • patch
  • minor
  • major

🐤 Download canary assets:

nexus--canary.272.4869012901.zip

📦 Published PR as canary version: 0.0.19--canary.272.4869012901.0

✨ Test out this PR locally via:

npm install @nexus-wallet/[email protected]
npm install @nexus-wallet/[email protected]
npm install @nexus-wallet/[email protected]
npm install @nexus-wallet/[email protected]
# or 
yarn add @nexus-wallet/[email protected]
yarn add @nexus-wallet/[email protected]
yarn add @nexus-wallet/[email protected]
yarn add @nexus-wallet/[email protected]

homura
homura previously approved these changes Apr 27, 2023
packages/ownership-providers/src/FullOwnershipProvider.ts Outdated Show resolved Hide resolved
@homura homura dismissed their stale review April 27, 2023 14:25

click the wrong button

@homura homura added the minor Increment the minor version when merged label Apr 27, 2023
@IronLu233 IronLu233 marked this pull request as ready for review April 28, 2023 10:06
@codecov
Copy link

codecov bot commented Apr 28, 2023

Codecov Report

Merging #272 (79e2e14) into main (23ca2e3) will increase coverage by 1.08%.
The diff coverage is 93.20%.

@@            Coverage Diff             @@
##             main     #272      +/-   ##
==========================================
+ Coverage   74.80%   75.88%   +1.08%     
==========================================
  Files          61       62       +1     
  Lines        1401     1497      +96     
  Branches      197      216      +19     
==========================================
+ Hits         1048     1136      +88     
- Misses        329      337       +8     
  Partials       24       24              
Impacted Files Coverage Δ
packages/ownership-providers/src/utils.ts 88.67% <88.67%> (ø)
...s/ownership-providers/src/FullOwnershipProvider.ts 97.23% <98.00%> (-0.59%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

Choose a reason for hiding this comment

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

we should mark all these functions as @internal

packages/ownership-providers/src/FullOwnershipProvider.ts Outdated Show resolved Hide resolved
packages/ownership-providers/src/FullOwnershipProvider.ts Outdated Show resolved Hide resolved
packages/ownership-providers/src/FullOwnershipProvider.ts Outdated Show resolved Hide resolved

await provider.sendTransaction(txSkeleton);

expect(provider.payFee).not.toHaveBeenCalled();
Copy link
Contributor

Choose a reason for hiding this comment

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

This test made me realize that we should ban transactions with fees above a threshold, as this is usually the case with errors. We can do it in another issue

Copy link
Contributor

@homura homura left a comment

Choose a reason for hiding this comment

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

LGTM

@homura homura merged commit 6c68a61 into main May 3, 2023
@IronLu233 IronLu233 deleted the ownershipProvider-sendTransaction-with-payFee branch May 4, 2023 02:09
@github-actions
Copy link

github-actions bot commented May 4, 2023

🚀 PR was released in v0.0.19 🚀

@github-actions github-actions bot added the released This issue/pull request has been released. label May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Increment the minor version when merged released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Encapsulate the sendTranasction method in the OwnershipProvider
2 participants