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/da 148 eliminate transaction queues #785

Merged
merged 32 commits into from
Aug 24, 2022

Conversation

monitz87
Copy link
Contributor

@monitz87 monitz87 commented Jul 4, 2022

Description

The TransactionQueue class has been removed. Most of its methods have been moved over to PolymeshTransaction and PolymeshTransactionBatch. Methods that returned a TransactionQueue (all endpoints that write to the blockchain, such as sdk.assets.registerTicker) now return either a PolymeshTransaction or PolymeshTransactionBatch. This means that all operations in the SDK are now atomic and not prone to race conditions or half-states

  • The onProcessedByMiddleware method has been moved to PolymeshTransaction and PolymeshTransactionBatch
  • The Fees interface now contains a total property which is the sum of free and locked
  • The PayingAccountType enum now includes a Caller member, to represent cases where the calling Account has to pay for a transaction’s fees

Breaking Changes

  • Remove the inputArgs property from PolymeshTransaction. Replaced by the args property. Arguments for PolymeshTransaction and PolymeshTransactionBatch are now available at any point in time (previously they could depend on the result of a previous transaction in the queue)
  • Remove the isCritical property from both PolymeshTransaction and PolymeshTransactionBatch. It no longer makes sense without Transaction Queues
  • The run method in PolymeshTransaction and PolymeshTransactionBatch now works similarly to the run method in TransactionQueue. The method will check if the caller account has enough balance for fees, the transaction will be run and its status updated. The method returns a Promise that resolves to the result of running the transaction. For example, calling run on the transaction returned by asset.registerTicker will return a Promise that resolves to a TickerReservation entity
  • Change the argument received by the callback passed to onStatusChange to PolymeshTransactionBase. If type refinement is required, the isPolymeshTransaction and isPolymeshTransactionBatch typeguards can be used (they can be imported from types)
  • Change the return type of getFees in PolymeshTransaction and PolymeshTransactionBatch to PayingAccountFees, which contains details about the Account that will pay for the fees, its remaining balance (before paying), and the fees themselves
  • Remove the getPayingAccount method from PolymeshTransaction and PolymeshTransactionBatch. Its result is now contained in getFees
  • The PayingAccount type now only contains allowance: BigNumber when type is Subsidy. For Caller and Other, there is no allowance

JIRA Link

https://polymath.atlassian.net/browse/DA-148?atlOrigin=eyJpIjoiZWEyYjE3ZDA1NzIxNDg3NmI2ZWNiNGMzNDlkMzBmMjAiLCJwIjoiaiJ9

Checklist

  • Updated the Readme.md (if required) ?

The `TransactionQueue` class has been removed. Most of its methods have been moved over to
`PolymeshTransaction` and `PolymeshTransactionBatch`. Methods that returned a `TransactionQueue`
(all endpoints that write to the blockchain, such as `sdk.assets.registerTicker`) now return
either a `PolymeshTransaction` or `PolymeshTransactionBatch`. This means that all operations
in the SDK are now atomic and not prone to race conditions or half-states

- The `onProcessedByMiddleware` method has been moved to `PolymeshTransaction` and
  `PolymeshTransactionBase`
- The `Fees` interface now contains a `total` property which is the sum of `free`
  and `locked`
- The `PayingAccountType` enum now includes a `Caller` member, to represent cases
  where the calling Account has to pay for a transaction’s fees

BREAKING CHANGES:

- Remove the `inputArgs` property from `PolymeshTransaction`. Replaced by the `args`
  property. Arguments for `PolymeshTransaction` and `PolymeshTransactionBatch` are now
  available at any point in time (previously they could depend on the result of a previous
  transaction in the queue)
- Remove the `isCritical` property from both `PolymeshTransaction` and `PolymeshTransactionBatch`.
  It no longer makes sense without Transaction Queues
- Change the `run` method in `PolymeshTransaction` and `PolymeshTransactionBatch` to work similarly
  to the `run` method in `TransactionQueue`. The method will check if the caller account has
  enough balance for fees, the transaction will be run and its status updated. The method returns
  a Promise that resolves to the result of running the transaction. For example, calling `run` on
  the transaction returned by `asset.registerTicker` will return a Promise that resolves to a
  `TickerReservation` entity
- Change the argument received by the callback passed to `onStatusChange` to
  `PolymeshTransactionBase`. If type refinement is required, the `isPolymeshTransaction`
  and `isPolymeshTransactionBatch` typeguards can be used (they can be imported from `types`)
- Change the return type of `getFees` in `PolymeshTransaction` and `PolymeshTransactionBatch`
  to `PayingAccountFees`, which contains details about the Account that will pay for the fees,
  its remaining balance (before paying), and the fees themselves
- Remove the `getPayingAccount` method from `PolymeshTransaction` and `PolymeshTransactionBatch`.
  Its result is now contained in `getFees`
- Change the `PayingAccount` type to now only contain `allowance: BigNumber` when `type` is
  `Subsidy`. For `Caller` and `Other`, there is no `allowance`
@monitz87 monitz87 marked this pull request as draft July 4, 2022 20:42
Copy link
Collaborator

@prashantasdeveloper prashantasdeveloper left a comment

Choose a reason for hiding this comment

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

LGTM. Just leaving a nit comment on newly added tests to reduce the same code duplication. (Sonar left some code smells as well)

Comment on lines 184 to 188
balance: {
free: new BigNumber(1000000),
locked: new BigNumber(0),
total: new BigNumber(1000000),
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit -> Can be defined as separate const and used here as well as in line 172 to remove duplication

}

/**
* Schema of a transaction batch
*
* @param Args - arguments of the transaction
* @param Values - values that will be returned wrapped in {@link PostTransactionValue} after the transaction runs
*/
export interface BatchTransactionSpec<
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see 9 examples of the comment 3 line comment starting with:

we use unknown[][] here because

Instead can we put the comment around unknown[][] as a @note here instead. It will have to be reworded, it can mention whats preferred, but that there is the 2d unknown array option for things not known at compile time and the types can be narrowed with checkTxType and assembleBatchTx

Having a large comment repeated seems messy to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having a large comment repeated seems messy to me

Agreed

@@ -96,9 +113,9 @@ export type TxWithArgs<Args extends unknown[] = unknown[]> = BaseTx<Args> &
}
: {
/**
* arguments that the transaction will receive (some of them can be {@link PostTransactionValue} from an earlier transaction)
* arguments that the transaction will receive
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need this comment? Seems redundant now

@@ -186,18 +205,26 @@ export interface BaseTransactionSpec<Values extends unknown[] = unknown[]> {
* they try to execute a transaction with `paidForBy` set, the fees will be paid for by the `paidForBy` Identity
*/
paidForBy?: Identity;
/**
* value that the transaction will yield once it has run, or a function that yields that value
Copy link
Collaborator

Choose a reason for hiding this comment

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

In english yield makes sense, but I don't think it's a good term to use in this context.

yield is a keyword in JS, so this implies the function is a generator, which isn't the case.

Suggested change
* value that the transaction will yield once it has run, or a function that yields that value
* value that the transaction will resolve to once it has run, or a callback function that returns the value

@@ -275,7 +275,12 @@ async function getTxArgsAndErrors(
export async function prepareAddInstruction(
this: Procedure<Params, Instruction[], Storage>,
args: Params
): Promise<PostTransactionValue<Instruction[]>> {
/*
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is repeated a lot. 2 ideas:

  • Default the param to unknown[][] so its not seen here
  • Add @note to BatchTransactionSpec about how to handle it

): Promise<PostTransactionValue<Instruction[]>> {
/*
* we use `unknown[][]` here because it's impossible to type this without knowing
* how many transactions will be in the batch beforehand. Type safety is ensured via
Copy link
Collaborator

Choose a reason for hiding this comment

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

Type safety

A nitty technical comment, but I see the term used a few places.

My understanding is unknown is always "type safe", since no operation is permitted on it, however its useless because of it.

The method is narrowing unknown to give something useful, but its introducing the risk of having unsound types so its introducing the opposite of type safety to give something thats actually usable.

My preference would be to find a better way to express this than have a copy paste comment repeated though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right

* Retrieve the Account that would pay fees for the transaction if it was run at this moment, as well as the total amount that can be
* charged to it (allowance) in case of a subsidy
*
* @note this value might change if, before running the transaction, the caller Account enters (or leaves)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* @note this value might change if, before running the transaction, the caller Account enters (or leaves)
* @note the actual fees charged may differ if, before running the transaction, the caller Account enters (or leaves)

"this value" as in PayingAccount is a constant so its not going to change

src/base/Procedure.ts Show resolved Hide resolved
});
sinon.assert.calledWith(context.setSigningAddress, 'something');

// const func2 = async function (
Copy link
Collaborator

Choose a reason for hiding this comment

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

either remove or uncomment

this.emitter.emit(Event.ProcessedByMiddleware);
return;
}
} catch (err) {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need this catch here? Wouldn't the .catch handle this error if it does happen? If there is a certain type of error this is meant, can it be scoped to the type at least.

I think its OK to leave in (its only one query), but generally I think swallowing every error silently can be frustrating to debug if there is an issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We swallow the error because if there's a problem running the query we just want to retry, not have the entire process fail

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add a comment explaining this

throw new PolymeshError({
code: ErrorCode.UnmetPrerequisite,
message:
"The subsidizer Account has not granted the caller Account enough allowance to pay this transaction's fees",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"The subsidizer Account has not granted the caller Account enough allowance to pay this transaction's fees",
"Insufficient subsidy allowance to pay this transaction's fees",

(nit) I think this can be less verbose

Copy link
Collaborator

@polymath-eric polymath-eric left a comment

Choose a reason for hiding this comment

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

LGTM!

const { number } = await this.polymeshApi.rpc.chain.getHeader();
const { chain } = this.polymeshApi.rpc;

const hash = await chain.getFinalizedHead();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wish we could use subscribeFinalizedHeads here since that returns the number, but needs a callback since its a subscription.

I think this is good enough although a double network call bothers me a little out of principle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm we could promisify the callback and save one call

Copy link
Collaborator

Choose a reason for hiding this comment

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

How does that work? I thought with a subscription you call out, and then next time a block gets finalized you get notified, vs the call where the response is right away. So it would take longer on average. It might be the case where the subscription gets the last value right away though.

bench mark the promisfy version if you try it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm almost sure subscriptions give you the current value right away as well as any future values. I'll try it out

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested it out and the callback is indeed called with the current value. I also benchmarked it and it's faster to use a promisified callback than to make 2 calls

monitz87 and others added 19 commits July 16, 2022 15:29
…nate-batch-events

Feat/da 174 discriminate batch events
feat: 🎸 Allow passing of `nonce` as part of `ProcedureOptions`
- Add an `sdk.createTransactionBatch` endpoint that takes an array of SDK transactions and returns
  them all batched in a single transaction. The result of running this batch is an array of the
  results of each transaction in the same order
- Fix documentation to reflect the fact that transaction queues no longer exist
- Improve transaction fee calculation algorithm to avoid edge cases where estimated fees were
  different than the actual fees
- Expose `getProtocolFees` from `PolymeshTransaction` and `PolymeshTransactionBatch`
- Expose the `GenericPolymeshTransaction` type
  (returned by most endpoints that create transactions)

BREAKING CHANGES:

- Rename `getFees` to `getTotalFees` in `PolymeshTransaction` and `PolymeshTransactionBatch`
Add a `splitTransactions` method to the `PolymeshTransactionBatch` class
that returns all individual transactions in the batch. This method is
useful when the caller is being subsidized, since batches do not support
subsidies
…transactions

Feat/da 176 merging transactions
…tch-transactions

feat: 🎸 support splitting batches
When a procedure that normally produces batches produced a single
transaction, we weren't passing manual fee data over
@monitz87 monitz87 marked this pull request as ready for review August 17, 2022 22:42
@polymath-eric
Copy link
Collaborator

polymath-eric commented Aug 23, 2022

fyi, I a pushed commit updating polymathnetwork -> polymeshassociation in the path. That should fix the test case that failed.

@sonarcloud
Copy link

sonarcloud bot commented Aug 24, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@polymath-eric polymath-eric merged commit 23af44e into alpha Aug 24, 2022
@polymath-eric polymath-eric deleted the feat/DA-148-eliminate-transaction-queues branch August 24, 2022 13:05
polymath-eric added a commit that referenced this pull request Aug 24, 2022
triggers release of latest alpha as the previous merge had older commits
skipped be semver bot

Feat/da 148 eliminate transaction queues (#785)

* feat: remove Transaction Queues

The `TransactionQueue` class has been removed. Most of its methods have been moved over to
`PolymeshTransaction` and `PolymeshTransactionBatch`. Methods that returned a `TransactionQueue`
(all endpoints that write to the blockchain, such as `sdk.assets.registerTicker`) now return
either a `PolymeshTransaction` or `PolymeshTransactionBatch`. This means that all operations
in the SDK are now atomic and not prone to race conditions or half-states

- The `onProcessedByMiddleware` method has been moved to `PolymeshTransaction` and
  `PolymeshTransactionBase`
- The `Fees` interface now contains a `total` property which is the sum of `free`
  and `locked`
- The `PayingAccountType` enum now includes a `Caller` member, to represent cases
  where the calling Account has to pay for a transaction’s fees

BREAKING CHANGES:

- Remove the `inputArgs` property from `PolymeshTransaction`. Replaced by the `args`
  property. Arguments for `PolymeshTransaction` and `PolymeshTransactionBatch` are now
  available at any point in time (previously they could depend on the result of a previous
  transaction in the queue)
- Remove the `isCritical` property from both `PolymeshTransaction` and `PolymeshTransactionBatch`.
  It no longer makes sense without Transaction Queues
- Change the `run` method in `PolymeshTransaction` and `PolymeshTransactionBatch` to work similarly
  to the `run` method in `TransactionQueue`. The method will check if the caller account has
  enough balance for fees, the transaction will be run and its status updated. The method returns
  a Promise that resolves to the result of running the transaction. For example, calling `run` on
  the transaction returned by `asset.registerTicker` will return a Promise that resolves to a
  `TickerReservation` entity
- Change the argument received by the callback passed to `onStatusChange` to
  `PolymeshTransactionBase`. If type refinement is required, the `isPolymeshTransaction`
  and `isPolymeshTransactionBatch` typeguards can be used (they can be imported from `types`)
- Change the return type of `getFees` in `PolymeshTransaction` and `PolymeshTransactionBatch`
  to `PayingAccountFees`, which contains details about the Account that will pay for the fees,
  its remaining balance (before paying), and the fees themselves
- Remove the `getPayingAccount` method from `PolymeshTransaction` and `PolymeshTransactionBatch`.
  Its result is now contained in `getFees`
- Change the `PayingAccount` type to now only contain `allowance: BigNumber` when `type` is
  `Subsidy`. For `Caller` and `Other`, there is no `allowance`

* perf: ⚡️ use a single call to fetch latest finalized block

* chore: 🤖 implement a way of discriminating batch events

* feat: 🎸 Allow passing of `nonce` as part of `ProcedureOptions`

* refactor: 💡 Remove unnecessary nonce value from txSpec

* feat: 🎸 Add `currentNonce` method to `Account`

* chore: 🤖 Allow async values for nonce

* Update src/types/index.ts

* feat: support merging transactions into batches

- Add an `sdk.createTransactionBatch` endpoint that takes an array of SDK transactions and returns
  them all batched in a single transaction. The result of running this batch is an array of the
  results of each transaction in the same order
- Fix documentation to reflect the fact that transaction queues no longer exist
- Improve transaction fee calculation algorithm to avoid edge cases where estimated fees were
  different than the actual fees
- Expose `getProtocolFees` from `PolymeshTransaction` and `PolymeshTransactionBatch`
- Expose the `GenericPolymeshTransaction` type
  (returned by most endpoints that create transactions)

BREAKING CHANGES:

- Rename `getFees` to `getTotalFees` in `PolymeshTransaction` and `PolymeshTransactionBatch`

* feat: 🎸 expose the `MapTxWithData` type

* docs: ✏️ improve rendered documentation to handle code snippets

* test: 💍 use proper type in assertion

* feat: 🎸 support splitting batches

Add a `splitTransactions` method to the `PolymeshTransactionBatch` class
that returns all individual transactions in the batch. This method is
useful when the caller is being subsidized, since batches do not support
subsidies

* fix: 🐛 carry manual fees over when simplifying a batch

When a procedure that normally produces batches produced a single
transaction, we weren't passing manual fee data over

BREAKING CHANGE: 🧨 no more transaction queues
polymath-eric added a commit that referenced this pull request Aug 24, 2022
triggers release of latest alpha as the previous merge had older commits
skipped be semver bot

Feat/da 148 eliminate transaction queues (#785)

* feat: remove Transaction Queues

The `TransactionQueue` class has been removed. Most of its methods have been moved over to
`PolymeshTransaction` and `PolymeshTransactionBatch`. Methods that returned a `TransactionQueue`
(all endpoints that write to the blockchain, such as `sdk.assets.registerTicker`) now return
either a `PolymeshTransaction` or `PolymeshTransactionBatch`. This means that all operations
in the SDK are now atomic and not prone to race conditions or half-states

- The `onProcessedByMiddleware` method has been moved to `PolymeshTransaction` and
  `PolymeshTransactionBase`
- The `Fees` interface now contains a `total` property which is the sum of `free`
  and `locked`
- The `PayingAccountType` enum now includes a `Caller` member, to represent cases
  where the calling Account has to pay for a transaction’s fees

* perf: ⚡️ use a single call to fetch latest finalized block

* chore: 🤖 implement a way of discriminating batch events

* feat: 🎸 Allow passing of `nonce` as part of `ProcedureOptions`

* refactor: 💡 Remove unnecessary nonce value from txSpec

* feat: 🎸 Add `currentNonce` method to `Account`

* chore: 🤖 Allow async values for nonce

* Update src/types/index.ts

* feat: support merging transactions into batches

* feat: 🎸 expose the `MapTxWithData` type

* docs: ✏️ improve rendered documentation to handle code snippets

* test: 💍 use proper type in assertion

* feat: 🎸 support splitting batches

* fix: 🐛 carry manual fees over when simplifying a batch

Add a `splitTransactions` method to the `PolymeshTransactionBatch` class
that returns all individual transactions in the batch. This method is
useful when the caller is being subsidized, since batches do not support
subsidies

When a procedure that normally produces batches produced a single
transaction, we weren't passing manual fee data over

- Add an `sdk.createTransactionBatch` endpoint that takes an array of SDK transactions and returns
  them all batched in a single transaction. The result of running this batch is an array of the
  results of each transaction in the same order
- Fix documentation to reflect the fact that transaction queues no longer exist
- Improve transaction fee calculation algorithm to avoid edge cases where estimated fees were
  different than the actual fees
- Expose `getProtocolFees` from `PolymeshTransaction` and `PolymeshTransactionBatch`
- Expose the `GenericPolymeshTransaction` type
  (returned by most endpoints that create transactions)

BREAKING CHANGES:

- Remove the `inputArgs` property from `PolymeshTransaction`. Replaced by the `args`
  property. Arguments for `PolymeshTransaction` and `PolymeshTransactionBatch` are now
  available at any point in time (previously they could depend on the result of a previous
  transaction in the queue)
- Remove the `isCritical` property from both `PolymeshTransaction` and `PolymeshTransactionBatch`.
  It no longer makes sense without Transaction Queues
- Change the `run` method in `PolymeshTransaction` and `PolymeshTransactionBatch` to work similarly
  to the `run` method in `TransactionQueue`. The method will check if the caller account has
  enough balance for fees, the transaction will be run and its status updated. The method returns
  a Promise that resolves to the result of running the transaction. For example, calling `run` on
  the transaction returned by `asset.registerTicker` will return a Promise that resolves to a
  `TickerReservation` entity
- Change the argument received by the callback passed to `onStatusChange` to
  `PolymeshTransactionBase`. If type refinement is required, the `isPolymeshTransaction`
  and `isPolymeshTransactionBatch` typeguards can be used (they can be imported from `types`)
- Change the return type of `getFees` in `PolymeshTransaction` and `PolymeshTransactionBatch`
  to `PayingAccountFees`, which contains details about the Account that will pay for the fees,
  its remaining balance (before paying), and the fees themselves
- Remove the `getPayingAccount` method from `PolymeshTransaction` and `PolymeshTransactionBatch`.
  Its result is now contained in `getFees`
- Change the `PayingAccount` type to now only contain `allowance: BigNumber` when `type` is
  `Subsidy`. For `Caller` and `Other`, there is no `allowance`
- Rename `getFees` to `getTotalFees` in `PolymeshTransaction` and `PolymeshTransactionBatch`
polymath-eric added a commit that referenced this pull request Aug 24, 2022
triggers release of latest alpha as the previous merge had older commits
skipped be semver bot

Feat/da 148 eliminate transaction queues (#785)

* feat: remove Transaction Queues

The `TransactionQueue` class has been removed. Most of its methods have been moved over to
`PolymeshTransaction` and `PolymeshTransactionBatch`. Methods that returned a `TransactionQueue`
(all endpoints that write to the blockchain, such as `sdk.assets.registerTicker`) now return
either a `PolymeshTransaction` or `PolymeshTransactionBatch`. This means that all operations
in the SDK are now atomic and not prone to race conditions or half-states

- The `onProcessedByMiddleware` method has been moved to `PolymeshTransaction` and
  `PolymeshTransactionBase`
- The `Fees` interface now contains a `total` property which is the sum of `free`
  and `locked`
- The `PayingAccountType` enum now includes a `Caller` member, to represent cases
  where the calling Account has to pay for a transaction’s fees

* perf: ⚡️ use a single call to fetch latest finalized block

* chore: 🤖 implement a way of discriminating batch events

* feat: 🎸 Allow passing of `nonce` as part of `ProcedureOptions`

* refactor: 💡 Remove unnecessary nonce value from txSpec

* feat: 🎸 Add `currentNonce` method to `Account`

* chore: 🤖 Allow async values for nonce

* Update src/types/index.ts

* feat: support merging transactions into batches

* feat: 🎸 expose the `MapTxWithData` type

* docs: ✏️ improve rendered documentation to handle code snippets

* test: 💍 use proper type in assertion

* feat: 🎸 support splitting batches

* fix: 🐛 carry manual fees over when simplifying a batch

Add a `splitTransactions` method to the `PolymeshTransactionBatch` class
that returns all individual transactions in the batch. This method is
useful when the caller is being subsidized, since batches do not support
subsidies

When a procedure that normally produces batches produced a single
transaction, we weren't passing manual fee data over

- Add an `sdk.createTransactionBatch` endpoint that takes an array of SDK transactions and returns
  them all batched in a single transaction. The result of running this batch is an array of the
  results of each transaction in the same order
- Fix documentation to reflect the fact that transaction queues no longer exist
- Improve transaction fee calculation algorithm to avoid edge cases where estimated fees were
  different than the actual fees
- Expose `getProtocolFees` from `PolymeshTransaction` and `PolymeshTransactionBatch`
- Expose the `GenericPolymeshTransaction` type
  (returned by most endpoints that create transactions)

BREAKING CHANGES:

- Remove the `inputArgs` property from `PolymeshTransaction`. Replaced by the `args`
  property. Arguments for `PolymeshTransaction` and `PolymeshTransactionBatch` are now
  available at any point in time (previously they could depend on the result of a previous
  transaction in the queue)
- Remove the `isCritical` property from both `PolymeshTransaction` and `PolymeshTransactionBatch`.
  It no longer makes sense without Transaction Queues
- Change the `run` method in `PolymeshTransaction` and `PolymeshTransactionBatch` to work similarly
  to the `run` method in `TransactionQueue`. The method will check if the caller account has
  enough balance for fees, the transaction will be run and its status updated. The method returns
  a Promise that resolves to the result of running the transaction. For example, calling `run` on
  the transaction returned by `asset.registerTicker` will return a Promise that resolves to a
  `TickerReservation` entity
- Change the argument received by the callback passed to `onStatusChange` to
  `PolymeshTransactionBase`. If type refinement is required, the `isPolymeshTransaction`
  and `isPolymeshTransactionBatch` typeguards can be used (they can be imported from `types`)
- Change the return type of `getFees` in `PolymeshTransaction` and `PolymeshTransactionBatch`
  to `PayingAccountFees`, which contains details about the Account that will pay for the fees,
  its remaining balance (before paying), and the fees themselves
- Remove the `getPayingAccount` method from `PolymeshTransaction` and `PolymeshTransactionBatch`.
  Its result is now contained in `getFees`
- Change the `PayingAccount` type to now only contain `allowance: BigNumber` when `type` is
  `Subsidy`. For `Caller` and `Other`, there is no `allowance`
- Rename `getFees` to `getTotalFees` in `PolymeshTransaction` and `PolymeshTransactionBatch`
@monitz87
Copy link
Contributor Author

🎉 This PR is included in version 15.0.0-alpha.17 🎉

The release is available on:

Your semantic-release bot 📦🚀

@monitz87
Copy link
Contributor Author

🎉 This PR is included in version 15.1.0-alpha.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

polymath-eric added a commit that referenced this pull request Sep 15, 2022
* feat: 🎸 allow for exempted identities to be removed

when a user does not pass exempted identities that are exempt when
calling setTransferRestrictions the procedure will unset those
permissions

BREAKING CHANGE: 🧨 non passed exempted identities are removed when calling
setTransferRestriction

* refactor: 💡 fix code smells

* test: 💍 add missing coverage

* test: 💍 clean up test assertions for restrictions

* test: 💍 add case for restriction type to stat type conversion

* refactor: 💡 address pr comments

* docs: ✏️ add comments to transformInput for set restrictions

* refactor: 💡 use function for pushing exemption transactions

* refactor: 💡 adjust exemption record structure

* docs: ✏️ add comments to addExemptionIfNotPresent method

* refactor: 💡 remove unused default

* style: 💄 add line break after sizeOf definition

* refactor: 💡 Make StatType internal as users never need it

The stat type is inferred from the TransferRestrictionType they are
interacting with. This eliminates the redundant StatisticsOpType and use
StatType through out

* docs: ✏️ add docs around StatType

* refactor: 💡 address PR comments

* refactor: 💡 use destructre for cleaner syntax

* Beta into alpha (#825)

* docs: ✏️ update compatible version and docs url in readme.md

* feat: 🎸 bump supported chain version to 5.0.2

* fix: 🐛 add warning to use the new npm repository

* chore: 🤖 update references to use association signing managers

* fix: 🐛 missing readme from npm

* fix: 🐛 update polymathnetwork to association references (#817)

* Update README.md

Co-authored-by: Prashant Bajpai <34747455+prashantasdeveloper@users.noreply.github.com>

* fix: 🐛 Fix inviteAccount method by updating the permission type (#821)

* fix: 🐛 Fix transfer with memo (#822)

use long type name when creating memos. Also replace lodash padEnd with
the built in function

Co-authored-by: Victor Vicente <VictorVicente@users.noreply.github.com>

* Merge pull request #818 from PolymeshAssociation/beta (#824)

Co-authored-by: Victor Vicente <VictorVicente@users.noreply.github.com>

Co-authored-by: Victor <victor.g.vicente@gmail.com>
Co-authored-by: Victor Vicente <VictorVicente@users.noreply.github.com>
Co-authored-by: Prashant Bajpai <34747455+prashantasdeveloper@users.noreply.github.com>

* Feat/da 148 eliminate transaction queues (#785)

* feat: remove Transaction Queues

The `TransactionQueue` class has been removed. Most of its methods have been moved over to
`PolymeshTransaction` and `PolymeshTransactionBatch`. Methods that returned a `TransactionQueue`
(all endpoints that write to the blockchain, such as `sdk.assets.registerTicker`) now return
either a `PolymeshTransaction` or `PolymeshTransactionBatch`. This means that all operations
in the SDK are now atomic and not prone to race conditions or half-states

- The `onProcessedByMiddleware` method has been moved to `PolymeshTransaction` and
  `PolymeshTransactionBase`
- The `Fees` interface now contains a `total` property which is the sum of `free`
  and `locked`
- The `PayingAccountType` enum now includes a `Caller` member, to represent cases
  where the calling Account has to pay for a transaction’s fees

BREAKING CHANGES:

- Remove the `inputArgs` property from `PolymeshTransaction`. Replaced by the `args`
  property. Arguments for `PolymeshTransaction` and `PolymeshTransactionBatch` are now
  available at any point in time (previously they could depend on the result of a previous
  transaction in the queue)
- Remove the `isCritical` property from both `PolymeshTransaction` and `PolymeshTransactionBatch`.
  It no longer makes sense without Transaction Queues
- Change the `run` method in `PolymeshTransaction` and `PolymeshTransactionBatch` to work similarly
  to the `run` method in `TransactionQueue`. The method will check if the caller account has
  enough balance for fees, the transaction will be run and its status updated. The method returns
  a Promise that resolves to the result of running the transaction. For example, calling `run` on
  the transaction returned by `asset.registerTicker` will return a Promise that resolves to a
  `TickerReservation` entity
- Change the argument received by the callback passed to `onStatusChange` to
  `PolymeshTransactionBase`. If type refinement is required, the `isPolymeshTransaction`
  and `isPolymeshTransactionBatch` typeguards can be used (they can be imported from `types`)
- Change the return type of `getFees` in `PolymeshTransaction` and `PolymeshTransactionBatch`
  to `PayingAccountFees`, which contains details about the Account that will pay for the fees,
  its remaining balance (before paying), and the fees themselves
- Remove the `getPayingAccount` method from `PolymeshTransaction` and `PolymeshTransactionBatch`.
  Its result is now contained in `getFees`
- Change the `PayingAccount` type to now only contain `allowance: BigNumber` when `type` is
  `Subsidy`. For `Caller` and `Other`, there is no `allowance`

* perf: ⚡️ use a single call to fetch latest finalized block

* chore: 🤖 implement a way of discriminating batch events

* feat: 🎸 Allow passing of `nonce` as part of `ProcedureOptions`

* refactor: 💡 Remove unnecessary nonce value from txSpec

* feat: 🎸 Add `currentNonce` method to `Account`

* chore: 🤖 Allow async values for nonce

* Update src/types/index.ts

* feat: support merging transactions into batches

- Add an `sdk.createTransactionBatch` endpoint that takes an array of SDK transactions and returns
  them all batched in a single transaction. The result of running this batch is an array of the
  results of each transaction in the same order
- Fix documentation to reflect the fact that transaction queues no longer exist
- Improve transaction fee calculation algorithm to avoid edge cases where estimated fees were
  different than the actual fees
- Expose `getProtocolFees` from `PolymeshTransaction` and `PolymeshTransactionBatch`
- Expose the `GenericPolymeshTransaction` type
  (returned by most endpoints that create transactions)

BREAKING CHANGES:

- Rename `getFees` to `getTotalFees` in `PolymeshTransaction` and `PolymeshTransactionBatch`

* feat: 🎸 expose the `MapTxWithData` type

* docs: ✏️ improve rendered documentation to handle code snippets

* test: 💍 use proper type in assertion

* feat: 🎸 support splitting batches

Add a `splitTransactions` method to the `PolymeshTransactionBatch` class
that returns all individual transactions in the batch. This method is
useful when the caller is being subsidized, since batches do not support
subsidies

* fix: 🐛 carry manual fees over when simplifying a batch

When a procedure that normally produces batches produced a single
transaction, we weren't passing manual fee data over

* test: 💍 polymathnetwork -> polymeshassociation in import path

Co-authored-by: Prashant Bajpai <prashant.bajpai19@gmail.com>
Co-authored-by: Victor Vicente <VictorVicente@users.noreply.github.com>
Co-authored-by: Prashant Bajpai <34747455+prashantasdeveloper@users.noreply.github.com>
Co-authored-by: Eric <ericrichardson@polymath.network>

* feat: 🎸 release elimination of transaction queues (#828)

triggers release of latest alpha as the previous merge had older commits
skipped be semver bot

Feat/da 148 eliminate transaction queues (#785)

* feat: remove Transaction Queues

The `TransactionQueue` class has been removed. Most of its methods have been moved over to
`PolymeshTransaction` and `PolymeshTransactionBatch`. Methods that returned a `TransactionQueue`
(all endpoints that write to the blockchain, such as `sdk.assets.registerTicker`) now return
either a `PolymeshTransaction` or `PolymeshTransactionBatch`. This means that all operations
in the SDK are now atomic and not prone to race conditions or half-states

- The `onProcessedByMiddleware` method has been moved to `PolymeshTransaction` and
  `PolymeshTransactionBase`
- The `Fees` interface now contains a `total` property which is the sum of `free`
  and `locked`
- The `PayingAccountType` enum now includes a `Caller` member, to represent cases
  where the calling Account has to pay for a transaction’s fees

* perf: ⚡️ use a single call to fetch latest finalized block

* chore: 🤖 implement a way of discriminating batch events

* feat: 🎸 Allow passing of `nonce` as part of `ProcedureOptions`

* refactor: 💡 Remove unnecessary nonce value from txSpec

* feat: 🎸 Add `currentNonce` method to `Account`

* chore: 🤖 Allow async values for nonce

* Update src/types/index.ts

* feat: support merging transactions into batches

* feat: 🎸 expose the `MapTxWithData` type

* docs: ✏️ improve rendered documentation to handle code snippets

* test: 💍 use proper type in assertion

* feat: 🎸 support splitting batches

* fix: 🐛 carry manual fees over when simplifying a batch

Add a `splitTransactions` method to the `PolymeshTransactionBatch` class
that returns all individual transactions in the batch. This method is
useful when the caller is being subsidized, since batches do not support
subsidies

When a procedure that normally produces batches produced a single
transaction, we weren't passing manual fee data over

- Add an `sdk.createTransactionBatch` endpoint that takes an array of SDK transactions and returns
  them all batched in a single transaction. The result of running this batch is an array of the
  results of each transaction in the same order
- Fix documentation to reflect the fact that transaction queues no longer exist
- Improve transaction fee calculation algorithm to avoid edge cases where estimated fees were
  different than the actual fees
- Expose `getProtocolFees` from `PolymeshTransaction` and `PolymeshTransactionBatch`
- Expose the `GenericPolymeshTransaction` type
  (returned by most endpoints that create transactions)

BREAKING CHANGES:

- Remove the `inputArgs` property from `PolymeshTransaction`. Replaced by the `args`
  property. Arguments for `PolymeshTransaction` and `PolymeshTransactionBatch` are now
  available at any point in time (previously they could depend on the result of a previous
  transaction in the queue)
- Remove the `isCritical` property from both `PolymeshTransaction` and `PolymeshTransactionBatch`.
  It no longer makes sense without Transaction Queues
- Change the `run` method in `PolymeshTransaction` and `PolymeshTransactionBatch` to work similarly
  to the `run` method in `TransactionQueue`. The method will check if the caller account has
  enough balance for fees, the transaction will be run and its status updated. The method returns
  a Promise that resolves to the result of running the transaction. For example, calling `run` on
  the transaction returned by `asset.registerTicker` will return a Promise that resolves to a
  `TickerReservation` entity
- Change the argument received by the callback passed to `onStatusChange` to
  `PolymeshTransactionBase`. If type refinement is required, the `isPolymeshTransaction`
  and `isPolymeshTransactionBatch` typeguards can be used (they can be imported from `types`)
- Change the return type of `getFees` in `PolymeshTransaction` and `PolymeshTransactionBatch`
  to `PayingAccountFees`, which contains details about the Account that will pay for the fees,
  its remaining balance (before paying), and the fees themselves
- Remove the `getPayingAccount` method from `PolymeshTransaction` and `PolymeshTransactionBatch`.
  Its result is now contained in `getFees`
- Change the `PayingAccount` type to now only contain `allowance: BigNumber` when `type` is
  `Subsidy`. For `Caller` and `Other`, there is no `allowance`
- Rename `getFees` to `getTotalFees` in `PolymeshTransaction` and `PolymeshTransactionBatch`

* feat: 🎸 use a more specific type for onStatusChange in txBase (#829)

use GenericPolymeshTransaction<ReturnValue, TransformedReturnValue> in
onStatusChange handler defined in PolymeshTransactionBase. This makes it
easier to define functions to be passed to it

* chore: 🤖 improve typing around `assembleBatchTransaction`

This reduces the need to use the `tuple` util, hopefully making the code
a bit more idiomatic

* fix: 🐛 fix bug where only the default signer was used (#831)

in Procedure `prepare` method, the passed in, non modified `context` was
used, when `ctx` was supposed to be used instead

* fix: 🐛 improve the typing of `splitTransactions`

The original types weren't being properly reflected by the compiler,
resulting in an array of only 2 transactions always. Also, when batching
transactions that had a transformed return value, the compiler was
throwing errors. Since we don't care about the original return value,
only the transformed one, using `any` is acceptable

* test: 💍 add missing coverage in setTransferRestriction

also remove unnessesary type assertions in createMock functions

* test: 💍 fix jest timer in PolymeshTransactionBase

* refactor: 💡 remove redundant await

* style: 💄 remove commented out code

Co-authored-by: Victor Vicente <VictorVicente@users.noreply.github.com>
Co-authored-by: Victor <victor.g.vicente@gmail.com>
Co-authored-by: Prashant Bajpai <34747455+prashantasdeveloper@users.noreply.github.com>
Co-authored-by: Jeremías Díaz <monitz87@gmail.com>
Co-authored-by: Prashant Bajpai <prashant.bajpai19@gmail.com>
@prashantasdeveloper
Copy link
Collaborator

🎉 This PR is included in version 17.0.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@prashantasdeveloper
Copy link
Collaborator

🎉 This PR is included in version 17.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

None yet

4 participants