Skip to content

feat(txm): transactions with value#647

Merged
GabrielMartinezRodriguez merged 3 commits intomasterfrom
gabriel/txm-value-parameter
Apr 28, 2025
Merged

feat(txm): transactions with value#647
GabrielMartinezRodriguez merged 3 commits intomasterfrom
gabriel/txm-value-parameter

Conversation

@GabrielMartinezRodriguez
Copy link
Contributor

@GabrielMartinezRodriguez GabrielMartinezRodriguez commented Apr 21, 2025

Description

This PR introduces a new feature in the TXM that allows creating transactions with a value. This enables making transfers.

Checklist

Basics

  • B1. I have applied the proper label & proper branch name (e.g. norswap/build-system-caching).
  • B2. This PR is not so big that it should be split & addresses only one concern.
  • B3. The PR targets the lowest branch it can (ideally master).

Reminder: PR review guidelines

Correctness

  • C1. Builds and passes tests.
  • C2. The code is properly parameterized & compatible with different environments (e.g. local,
    testnet, mainnet, standalone wallet, ...).
  • C3. I have manually tested my changes & connected features.

< INDICATE BROWSER, DEMO APP & OTHER ENV DETAILS USED FOR TESTING HERE >

< INDICATE TESTED SCENARIOS (USER INTERFACE INTERACTION, CODE FLOWS) HERE >

  • C4. I have performed a thorough self-review of my code after submitting the PR,
    and have updated the code & comments accordingly.

Architecture & Documentation

  • D1. I made it easy to reason locally about the code, by (1) using proper abstraction boundaries,
    (2) commenting these boundaries correctly, (3) adding inline comments for context when needed.
  • D2. All public-facing APIs & meaningful (non-local) internal APIs are properly documented in code
    comments.
  • D3. If appropriate, the general architecture of the code is documented in a code comment or
    in a Markdown document.
  • D4. An appropriate Changeset has been generated (and committed) for changes that touch npm published packages (currently packages/core and packages/react), see here for more info.

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Apr 21, 2025

Deploying happychain with  Cloudflare Pages  Cloudflare Pages

Latest commit: c4fea51
Status:⚡️  Build in progress...

View logs

Copy link
Contributor Author

GabrielMartinezRodriguez commented Apr 21, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

@GabrielMartinezRodriguez GabrielMartinezRodriguez added the draft Not ready for review label Apr 21, 2025
@GabrielMartinezRodriguez GabrielMartinezRodriguez marked this pull request as ready for review April 22, 2025 13:27
@GabrielMartinezRodriguez GabrielMartinezRodriguez added reviewing-1 Ready for, or undergoing first-line review and removed draft Not ready for review labels Apr 22, 2025
Copy link
Contributor

@not-reed not-reed left a comment

Choose a reason for hiding this comment

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

i think in the txm getting-started changing /txm/api/interfaces/TransactionConstructorConfig to /txm/api/type-aliases/TransactionConstructorConfig fixes the docs build error

"account-abstraction": "https://github.com/eth-infinitism/account-abstraction#v0.7.0",
"forge-std": "https://github.com/foundry-rs/forge-std.git#v1.9.6",
"kernel": "https://github.com/zerodevapp/kernel#v3.1",
"node-jq": "^6.0.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

why the move here?

@GabrielMartinezRodriguez GabrielMartinezRodriguez mentioned this pull request Apr 23, 2025
11 tasks
address: this.address,
functionName: this.functionName,
contractName: this.contractName,
value: bigIntToZeroPadded(this.value), // We convert the bigint value to a zero-padded string because 'value' can exceed the numeric limits of Number
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure why we need the zero padding here actually? Couldn't we just this.value.toString()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Padding is needed to ensure that when we compare two strings representing bigints, the comparisons work correctly. We're not comparing numbers but strings, so without padding, the comparison would be alphabetical. For example, "9" would be considered greater than "10" because alphabetically, "9" comes after "1"

@norswap norswap added merge-after-changes Can be merged after requested changes are made and removed reviewing-1 Ready for, or undergoing first-line review labels Apr 24, 2025
@norswap
Copy link
Collaborator

norswap commented Apr 24, 2025

Just need to revert the node-jq change + possibly get rid of the padding.

@GabrielMartinezRodriguez GabrielMartinezRodriguez force-pushed the gabriel/transactions-with-calldata branch from 69f4e43 to 57343bb Compare April 24, 2025 10:03
@GabrielMartinezRodriguez GabrielMartinezRodriguez force-pushed the gabriel/transactions-with-calldata branch from 57343bb to 4361ec6 Compare April 24, 2025 10:12
@GabrielMartinezRodriguez GabrielMartinezRodriguez force-pushed the gabriel/txm-value-parameter branch 2 times, most recently from 059fedb to bfbf5b3 Compare April 24, 2025 10:26
@GabrielMartinezRodriguez GabrielMartinezRodriguez force-pushed the gabriel/transactions-with-calldata branch from b6848f9 to 7e3bd09 Compare April 24, 2025 11:12
@GabrielMartinezRodriguez GabrielMartinezRodriguez added merge-blocked Ready to merge, waiting for downstack and removed merge-after-changes Can be merged after requested changes are made labels Apr 24, 2025
@GabrielMartinezRodriguez GabrielMartinezRodriguez force-pushed the gabriel/transactions-with-calldata branch from 7e3bd09 to 9ba69a6 Compare April 28, 2025 11:08
@GabrielMartinezRodriguez GabrielMartinezRodriguez force-pushed the gabriel/transactions-with-calldata branch from 9ba69a6 to a081c22 Compare April 28, 2025 12:54
@GabrielMartinezRodriguez GabrielMartinezRodriguez force-pushed the gabriel/transactions-with-calldata branch from a081c22 to 1a07492 Compare April 28, 2025 12:57
Base automatically changed from gabriel/transactions-with-calldata to master April 28, 2025 12:57
@GabrielMartinezRodriguez GabrielMartinezRodriguez merged commit 2e5fc11 into master Apr 28, 2025
1 of 2 checks passed
@GabrielMartinezRodriguez GabrielMartinezRodriguez deleted the gabriel/txm-value-parameter branch April 28, 2025 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-blocked Ready to merge, waiting for downstack

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants