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!: transfer for multiple addresses #2333

Merged
merged 33 commits into from
May 21, 2024
Merged

Conversation

Torres-ssf
Copy link
Contributor

@Torres-ssf Torres-ssf commented May 17, 2024

Breaking Changes:

  • Changed BaseInvocationScope.addTransfer parameters

maschad
maschad previously approved these changes May 17, 2024
Copy link
Member

@maschad maschad left a comment

Choose a reason for hiding this comment

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

Awesome job on this 🚀 - just left one suggestion

apps/docs/src/guide/wallets/wallet-transferring.md Outdated Show resolved Hide resolved
Copy link
Contributor

@danielbate danielbate left a comment

Choose a reason for hiding this comment

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

This is a great feature and adds a lot of value. My only questions is around the spec in #161.

Also this should be integrated to enable queue txs instead of single tx

I can see we are using a single tx here, should we be queueing?

@Torres-ssf Torres-ssf changed the title feat: transfer for multiple addresses feat!: transfer for multiple addresses May 20, 2024
@fuel-service-user
Copy link
Contributor

fuel-service-user commented May 20, 2024

✨ A PR has been created under the rc-2333 tag on fuels-wallet repo.
FuelLabs/fuels-wallet#1311

@Torres-ssf
Copy link
Contributor Author

This is a great feature and adds a lot of value. My only questions is around the spec in #161.

Also this should be integrated to enable queue txs instead of single tx

I can see we are using a single tx here, should we be queueing?

@danielbate I reckon you were mentioning something like this: a56d59b, right?

Copy link
Member

@Dhaiwat10 Dhaiwat10 left a comment

Choose a reason for hiding this comment

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

Great work 🚀

@danielbate
Copy link
Contributor

@danielbate I reckon you were mentioning something like this: a56d59b, right?

But this still uses a single tx? A single transaction request with multiple recipient outputs. Whereas this issue also asks for:

queuing of transactions (i.e. once X tx is done, do X + 1, etc)

@Torres-ssf
Copy link
Contributor Author

But this still uses a single tx? A single transaction request with multiple recipient outputs. Whereas this issue also asks for:

queuing of transactions (i.e. once X tx is done, do X + 1, etc)

@danielbate Thanks for your explanation. I see your point.

I've created a specific issue for this since it's a completely different thing from multiple addresses transferring.

@Torres-ssf Torres-ssf enabled auto-merge (squash) May 21, 2024 11:43
Copy link
Contributor

Coverage Report:

Lines Branches Functions Statements
79.73%(+0%) 70.64%(-0.03%) 76.81%(-0.01%) 79.81%(+0%)
Changed Files:
Ok File (✨=New File) Lines Branches Functions Statements
🔴 packages/account/src/account.ts 84.14%
(+1.36%)
67.16%
(+0.5%)
80.64%
(+4.64%)
83.73%
(+1.38%)
🔴 packages/account/src/providers/transaction-request/transaction-request.ts 87.32%
(+1.41%)
77.63%
(+2.63%)
86%
(+0%)
87.67%
(+1.37%)

@Torres-ssf Torres-ssf merged commit 7c08593 into master May 21, 2024
19 checks passed
@Torres-ssf Torres-ssf deleted the st/feat/multi-transfer branch May 21, 2024 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Issue is a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transfer for multiple address at once
6 participants