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: batch transactions #72

Merged
merged 24 commits into from
Mar 18, 2022
Merged

feat: batch transactions #72

merged 24 commits into from
Mar 18, 2022

Conversation

ChaoticTempest
Copy link
Member

addresses: #67.

This PR adds a new Transaction type that allows us to add in actions into it, to form a batched transaction. A lot of stuff such as client.rs can use it directly instead of making individual calls, so that it's consistent throughout, but will save that for later in another PR.

One thing to note is that batch_tx.call() takes in a CallArgs type just so we can be consistent with the account.call().args().deposit()... API, and not need to specify optional parameters such as gas/deposit. This is how it would look like when adding calls into a batch transaction:

account.batch_tx(&worker, contract_id)
    .call(CallArgs::new("function")
        .args(...)
        .deposit(0)
    )
    ...
    .transact()
    .await?;

Not sure if this API is the best, or if there's a better naming than batch_tx. Let me know if there is.

Also considered the following API for .call but probably is too funky. Also would require a trait to be exposed in the prelude:

.call("function_name".into_call_args()
    .args(...)
    .deposit(...)
)

Copy link
Contributor

@itegulov itegulov left a comment

Choose a reason for hiding this comment

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

LGTM in general, just some naming nitpicking for now. I will make an attempt to use it in some of the examples later in the week and let you know my feedback.

Comment on lines 15 to 26
contract
.batch_tx(&worker)
.call(
CallArgs::new("set_status")
.args_json(json!({
"message": "hello_world",
}))?
.deposit(0),
)
.call(CallArgs::new("set_status").args_json(json!({
"message": "world_hello",
}))?)
Copy link
Contributor

Choose a reason for hiding this comment

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

The stylistic discrepancy between these two calls looks pretty weird, but I assume this is because of rustfmt...

In general I am not a big fan of how this "reads", but at the same time I can't come up with anything better right now.

workspaces/src/network/account.rs Outdated Show resolved Hide resolved
workspaces/src/network/transaction.rs Show resolved Hide resolved
workspaces/src/network/transaction.rs Outdated Show resolved Hide resolved
workspaces/tests/batch_tx.rs Outdated Show resolved Hide resolved
workspaces/src/network/transaction.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@miraclx miraclx left a comment

Choose a reason for hiding this comment

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

lgtm.

workspaces/src/network/transaction.rs Outdated Show resolved Hide resolved
workspaces/src/network/transaction.rs Outdated Show resolved Hide resolved
workspaces/src/network/transaction.rs Show resolved Hide resolved
Copy link
Contributor

@austinabell austinabell left a comment

Choose a reason for hiding this comment

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

just a quick scan for now. Will dig more into it later since it seems like there can be some duplication removed, but I need to play around with it to see what is possible

workspaces/src/network/transaction.rs Outdated Show resolved Hide resolved
workspaces/src/types.rs Outdated Show resolved Hide resolved
workspaces/src/network/transaction.rs Outdated Show resolved Hide resolved
workspaces/src/network/transaction.rs Show resolved Hide resolved
workspaces/src/network/transaction.rs Outdated Show resolved Hide resolved
workspaces/src/network/transaction.rs Show resolved Hide resolved
workspaces/src/types.rs Outdated Show resolved Hide resolved
workspaces/tests/batch_tx.rs Show resolved Hide resolved
workspaces/tests/batch_tx.rs Show resolved Hide resolved
@ChaoticTempest ChaoticTempest merged commit af2e8a8 into main Mar 18, 2022
@ChaoticTempest ChaoticTempest deleted the feat/batch-tx branch March 18, 2022 21:30
@frol frol mentioned this pull request Oct 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants