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

[Blocked] Update TransactionRequest's to field to TxKind #553

Merged
merged 20 commits into from
Apr 23, 2024

Conversation

EmperorOrokuSaki
Copy link
Contributor

@EmperorOrokuSaki EmperorOrokuSaki commented Apr 16, 2024

Motivation

Issue #549

Solution

Note: This is not the full refactor, I'll mark it ready for review once it's done :)
From the issue:

[X] Modify to field to make creates explicit
[X] Error on non-4844 build if to is unset
[X] Error on 4844 build if to is unset or create
[X] Add following to the builder trait

    - [X]  fn as_create(self) -> Self shortcut to set the field to Some(Create)

    - [X] fn deploy_code(self, code: Vec<u8>) -> Self to set data and to fields at once

    - [X] fn with_call<T: SolCall>(self, t: T) -> Self to set data and ensure to is NOT Create (set to to some if it is create)

PR Checklist

  • Added Tests
  • Added Documentation
  • [ X] Breaking changes

Copy link
Member

@prestwich prestwich left a comment

Choose a reason for hiding this comment

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

looking good overall. need to add a to check to fn check_reqd_fields as well

@prestwich prestwich marked this pull request as ready for review April 18, 2024 11:23
@prestwich prestwich changed the title Refactor [WIP]: Update TransactionRequest's to field to TxKind [Blocked] Update TransactionRequest's to field to TxKind Apr 18, 2024
@prestwich
Copy link
Member

prestwich commented Apr 18, 2024

This is now blocked by sol! expansion changes that i can work on soon

@prestwich
Copy link
Member

mututally dependent with alloy-rs/core#606

@@ -266,7 +266,7 @@ mod tests {
let tx = TransactionRequest {
from: Some(anvil.addresses()[0]),
value: Some(U256::from(100)),
to: address!("d8dA6BF26964aF9D7eEd9e03E53415D37aA96045").into(),
to: Some(address!("d8dA6BF26964aF9D7eEd9e03E53415D37aA96045").into()),
Copy link
Member

@gakonst gakonst Apr 19, 2024

Choose a reason for hiding this comment

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

should these be using TransactionBuilder instead of defining the transaction fully?

Copy link
Member

Choose a reason for hiding this comment

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

yeah but its just tests so i didn't want to clutter the diff

@prestwich
Copy link
Member

prestwich commented Apr 20, 2024

currently blocked by updating to a version of alloy-core with the new sol! expansion

@gakonst
Copy link
Member

gakonst commented Apr 23, 2024

Unblocked now alloy-rs/core@9ff1e5a -- @DaniPopes should we cut core release?

@DaniPopes DaniPopes merged commit e771b09 into alloy-rs:main Apr 23, 2024
18 checks passed
ben186 pushed a commit to ben186/alloy that referenced this pull request Jul 27, 2024
)

* feat: change the `to` field type to TxKind, update From<TxEip4844> accordingly

* refactor: update builder functions to include `to` checks.

* feat: add as_create(), with_call(), and deploy_code()

* refactor: remove error lines

* feat: with_call impl.

* feat: add the methods to the builder trait, update code that was broken.

* test: update tests.

* nits: reordering and default impls

* fix: some test compilation

* nit: fmt

* lint: clippy

* test: fix tests

* refactor: kind vs to

* feat: new_raw_deploy and generic input

* doc: add note re sol usage

* fix: type inference in test

* chore: update core

* docs

---------

Co-authored-by: James <james@prestwi.ch>
Co-authored-by: DaniPopes <57450786+DaniPopes@users.noreply.github.com>
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