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.

Sign up for GitHub

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

Jump to bottom

subcall wrapper + unit tests, and staking address generation #170

Merged
merged 15 commits into from
Sep 18, 2023
Merged

subcall wrapper + unit tests, and staking address generation #170

merged 15 commits into from
Sep 18, 2023

Conversation

CedarMist
Copy link
Member

@CedarMist CedarMist commented Aug 29, 2023 •
edited
Loading

This implements the following runtime-sdk functions:

  • consensus.Undelegate
  • consensus.Delegate
  • consensus.Withdraw
  • accounts.Transfer

It also supports generation of staking wallet addresses. via Subcall.sol:ConsensusUtils.generateStakingAddress

consensus.Deposit isn't supported.

I will need to update the documentation to include some subcall examples using the Subcall.sol library.

Subcall is no-longer in Sapphire.sol, all the functionality for subcalls is implemented in Subcall.sol.

@CedarMist CedarMist added s:underway Status: a patch is in progress s:blocked Status: work on this issue is blocked contracts Pull requests that update sapphire-contracts labels Aug 29, 2023
@CedarMist CedarMist marked this pull request as ready for review September 5, 2023 08:52
@CedarMist CedarMist removed s:blocked Status: work on this issue is blocked s:underway Status: a patch is in progress labels Sep 5, 2023
@CedarMist CedarMist requested review from matevz and kostko September 5, 2023 11:40
matevz
matevz requested changes Sep 5, 2023
View reviewed changes
contracts/contracts/Subcall.sol Outdated Show resolved Hide resolved
contracts/contracts/Subcall.sol Show resolved Hide resolved
contracts/contracts/Subcall.sol Outdated Show resolved Hide resolved
contracts/contracts/Subcall.sol Outdated Show resolved Hide resolved
contracts/contracts/Subcall.sol Outdated Show resolved Hide resolved
contracts/contracts/Subcall.sol Outdated Show resolved Hide resolved
contracts/test/subcall.ts Show resolved Hide resolved
contracts/test/subcall.ts Show resolved Hide resolved
contracts/test/subcall.ts Show resolved Hide resolved
contracts/test/subcall.ts Show resolved Hide resolved
@CedarMist CedarMist requested a review from matevz September 5, 2023 13:50
matevz
matevz requested changes Sep 5, 2023
View reviewed changes
Copy link
Member

@matevz matevz left a comment

Choose a reason for hiding this comment

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

Please rename StakingPublicKey to StakingAddress. Also let's use term "address" for destination address instead of "account" for delegation/undelegation/withdrawal.

contracts/contracts/Subcall.sol Outdated Show resolved Hide resolved
contracts/contracts/Subcall.sol Outdated Show resolved Hide resolved
contracts/contracts/Subcall.sol Outdated Show resolved Hide resolved
contracts/contracts/Subcall.sol Outdated Show resolved Hide resolved
contracts/contracts/Subcall.sol Outdated Show resolved Hide resolved
@CedarMist
Copy link
Member Author

Please rename StakingPublicKey to StakingAddress. Also let's use term "address" for destination address instead of "account" for delegation/undelegation/withdrawal.

I'm not sure why I used public key, I even had a function called derive address from public key...

@CedarMist CedarMist changed the title subcall examples subcall wrapper + unit tests, and staking address generation Sep 5, 2023
@CedarMist CedarMist force-pushed the CedarMist/subcall-examples branch from 180b363 to 95e714c Compare September 14, 2023 12:18
@CedarMist CedarMist requested a review from matevz September 14, 2023 12:28
matevz
matevz requested changes Sep 15, 2023
View reviewed changes
Copy link
Member

@matevz matevz left a comment

Choose a reason for hiding this comment

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

Please document all external, public and internal functions and constants in Solidity code since that's what the docs will be generated of. Otherwise looks ok, will make another pass later.

contracts/contracts/ConsensusUtils.sol Show resolved Hide resolved
@CedarMist CedarMist requested a review from matevz September 17, 2023 10:16
matevz
matevz approved these changes Sep 18, 2023
View reviewed changes
Copy link
Member

@matevz matevz left a comment

Choose a reason for hiding this comment

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

Some minor stylstic nits, otherwise LGTM.

contracts/contracts/ConsensusUtils.sol Outdated Show resolved Hide resolved
contracts/contracts/ConsensusUtils.sol Outdated Show resolved Hide resolved
contracts/contracts/Subcall.sol Outdated Show resolved Hide resolved
contracts/contracts/Subcall.sol Outdated Show resolved Hide resolved
contracts/contracts/Subcall.sol Outdated Show resolved Hide resolved
contracts/contracts/Subcall.sol Outdated Show resolved Hide resolved
contracts/contracts/Subcall.sol Outdated Show resolved Hide resolved
contracts/contracts/Subcall.sol Outdated Show resolved Hide resolved
contracts/test/subcall.ts Outdated Show resolved Hide resolved
contracts/contracts/Subcall.sol Outdated Show resolved Hide resolved
@CedarMist CedarMist force-pushed the CedarMist/subcall-examples branch from a70ea0b to b49404e Compare September 18, 2023 14:21
@CedarMist CedarMist merged commit f7f22cd into main Sep 18, 2023
16 checks passed
@CedarMist CedarMist deleted the CedarMist/subcall-examples branch September 18, 2023 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matevz matevz matevz approved these changes

@kostko kostko Awaiting requested review from kostko

Assignees

@aefhm aefhm

@CedarMist CedarMist

Labels
contracts Pull requests that update sapphire-contracts
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@CedarMist @matevz @aefhm