-
Notifications
You must be signed in to change notification settings - Fork 5
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: Split transaction builders #2
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Adding GitHub workflows
update readme
ilblackdragon
approved these changes
Aug 28, 2024
EdsonAlcala
changed the title
Split transaction builders
feat: Split transaction builders
Oct 2, 2024
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds several changes to the current code base, in my opinion there are some benefits that are described in more detail below, while it is also preserving the current API, with some minor changes.
Here is the description of the changes made:
Included rust config files:
Included justfile
Included licenses
Included Github workflows
Split transaction builders
The Near transaction building was abstracted into its own file(builder), called NearTransactionBuilder, this has the advantage of having the builder logic in a single place with the required fields for a Near transaction. This can be beneficial because with the current approach, users can fill fields that are not required for a Near Transaction and can be confusing.
The EVM Transaction building was abstracted into its own file too, called EVMTransactionBuilder. This share the same benefits of the NearTransactionBuilder, including users only filling what is required for an EVM transaction.
The NearTransactionBuilder now "builds" a NearTransaction type, this structure was included to provide additional capabilities like encoding with signature or simply encoding. The idea behind this is to allow users to not only encode transactions but also to enable the integration with the MPC signer and encode the signature within the smart contract. Facilitating the encoding and signature will allow users to simple request a signed transaction that can be easily propagated. The PR includes the signature encoding for the EVMTransaction, however a future PR will include the support for the NearTransaction. This change was required since the current implementation only returns a vec, by abstracting this into its own struct we can do this like:
Similar with Near:
The transaction builders now enforce calling the
build
function. Since this is the only way where the Transaction types are created (EVMTransaction or NearTransaction and more in the future). Facilitating their implementation and removing posible errors from implementers.Included integration tests for EVMTransaction against Anvil.
Included unit tests of encoding against alloy.
Included unit tests of encoding against Near primitives.
Removed Near primitives dependency.
Removed Near crypto dependency.
Limitations
A limitation of this current PR is the lack of support for the priority fee that was included in NEP-541: Transaction priority fee NEPs#541. This will be included in a future PR.
This PR only supports EIP 1559 transaction types. For support of legacy transactions, please share your thoughts.
Future Work
[ ] Include integration tests for Near
[ ] Support building with signature for NearTransaction
[ ] Create contribution guidelines
[ ] Create code of conduct
[ ] Create changelog
[ ] Add release workflow to publish the library as a crate
[ ] Write documentation
[ ] Document the code base
[ ] Create a pull request template and issue template
[ ] Test the following Near actions:
[ ] Support more Near actions:
[ ] Support priority fee in Near transactions, review near/NEPs#541
[ ] Create a examples repository with a structure as: