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

Wasm-JS interactivity, part two: staking roundtrip #1350

Merged
merged 3 commits into from
Feb 21, 2023

Conversation

sisou
Copy link
Member

@sisou sisou commented Feb 19, 2023

What's in this pull request?

Makes it possible to easily create and sign all transactions necessary to start staking, add stake, update delegation, and unstake.

  • Add creation methods for different transaction types
    • Introduces a TransactionBuilder class that is instantiated from the client and receives the client's network_id and a blockchain proxy (to access the current head block number), allowing users to omit these when creating transactions.
    • Adds a tx.sign(keyPair) method that automatically signs the transaction with the correct signature(s) format if possible.
    • Adds a constructor function to Transaction to allow the creation of arbitrary transactions which the transaction builder does not yet cover.
  • Adapt Client and Configuration to feel closer to the core-js client API.
  • Fix rustdocs

This relates to #1339.

Pull request checklist

  • All tests pass. The project builds and runs.
  • I have resolved any merge conflicts.
  • I have resolved all clippy and rustfmt warnings.

@sisou sisou added the WASM label Feb 19, 2023
@sisou sisou added this to the Nimiq 2.0 Testnet milestone Feb 19, 2023
@sisou sisou self-assigned this Feb 19, 2023
@codecov
Copy link

codecov bot commented Feb 20, 2023

Codecov Report

Base: 66.93% // Head: 66.59% // Decreases project coverage by -0.34% ⚠️

Coverage data is based on head (303294c) compared to base (6098449).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files
@@              Coverage Diff              @@
##           albatross    #1350      +/-   ##
=============================================
- Coverage      66.93%   66.59%   -0.34%     
=============================================
  Files            407      408       +1     
  Lines          51558    51796     +238     
=============================================
- Hits           34508    34494      -14     
- Misses         17050    17302     +252     
Flag Coverage Δ
unittests 66.59% <0.00%> (-0.34%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
web-client/src/lib.rs 0.46% <0.00%> (-0.03%) ⬇️
web-client/src/transaction.rs 0.00% <0.00%> (ø)
web-client/src/transaction_builder.rs 0.00% <0.00%> (ø)
handel/src/todo.rs 84.12% <0.00%> (-3.18%) ⬇️
network-mock/src/network.rs 83.90% <0.00%> (-2.20%) ⬇️
...twork-libp2p/src/discovery/message_codec/reader.rs 68.75% <0.00%> (-0.79%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@jsdanielh jsdanielh left a comment

Choose a reason for hiding this comment

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

Looks good. Just some minor comments

web-client/src/transaction.rs Outdated Show resolved Hide resolved
web-client/src/transaction.rs Outdated Show resolved Hide resolved
web-client/src/transaction.rs Outdated Show resolved Hide resolved
web-client/src/transaction_builder.rs Outdated Show resolved Hide resolved
web-client/src/transaction_builder.rs Show resolved Hide resolved
@viquezclaudio
Copy link
Member

LGTM

@sisou
Copy link
Member Author

sisou commented Feb 21, 2023

Thanks @jsdanielh, I missed those import orderings! I also applied all your other suggestions 👍

@sisou sisou mentioned this pull request Feb 21, 2023
10 tasks
@jsdanielh
Copy link
Member

@sisou I will be rebasing it to get it merged

- Introduces a TransactionBuilder class that is instantiated from the client and receives the client's networkId and blockchain proxy, to access the current head block number, allowing users to omit these when creating transactions.
- Adds a `sign(keyPair)` method on `Transaction` that automatically signs the transaction with the correct signature(s) format.
- Add a constructor function to `Transaction` to allow creating arbitrary transactions which are not yet covered by the transaction builder.
@jsdanielh jsdanielh merged commit 303294c into albatross Feb 21, 2023
@jsdanielh jsdanielh deleted the soeren/wasm-ext branch February 21, 2023 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants