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

Proof of concept exported API for Account actor #797

Merged
merged 1 commit into from
Nov 6, 2022

Conversation

anorth
Copy link
Member

@anorth anorth commented Oct 31, 2022

This is an example mechanism for defining and enforcing an exported API for builtin actors. The existing low-valued method numbers are all restricted to built-in callers only. This means we can, if ever needed, change their parameters or behaviour while preserving compatibility for callers of exported method numbers. But for now exported method numbers map to the same implementation as the internal.

Before merging:

  • Decide the right target branch for this work

Copy link
Contributor

@ZenGround0 ZenGround0 left a comment

Choose a reason for hiding this comment

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

I like this approach

@Stebalien
Copy link
Member

Ok, in general I like the idea of reserving a range for "internal" use. Comments:

  • It's strange that off-chain accounts can call "internal" methods. Are we going to change things like ProveCommit to use exported IDs? Or are we going to continue using "internal" methods from off-chain, but "exported" methods on-chain?
  • The whole "Exported" naming is kind of annoying, maybe create a separate table and/or enum? Then we can clearly label these as "private" and "exported" (with entirely separate enums).

@Stebalien
Copy link
Member

Also note: if we define a separate table, we can use a proc macro to compute the method name automatically:

#[frc42_methods]
pub enum MyMethods {
    Receive,
}

@arajasek
Copy link
Contributor

arajasek commented Nov 2, 2022

@Stebalien Thanks for the review!

  • It's strange that off-chain accounts can call "internal" methods. Are we going to change things like ProveCommit to use exported IDs? Or are we going to continue using "internal" methods from off-chain, but "exported" methods on-chain?

I think off-chain / on-chain isn't the relevant division here -- builtin vs. not-builtin is what matters. It's fine for off-chain entities to call "internal" methods, so long as the calling entity in question is a builtin actor (eg. the account actor). The current proposal likely won't export things like ProveCommit for now.

  • The whole "Exported" naming is kind of annoying, maybe create a separate table and/or enum? Then we can clearly label these as "private" and "exported" (with entirely separate enums).

This seems like a good suggestion, I think.

@anorth
Copy link
Member Author

anorth commented Nov 4, 2022

Eventually, I expect anything that an Account can call will become exported.

I toyed briefly with an alternative Enum, but it makes everything else more complicated and in some cases less transparent. Here the churn is confined to the enum values. Of course we can try other concrete approaches too.

@anorth anorth marked this pull request as ready for review November 4, 2022 02:09
@anorth
Copy link
Member Author

anorth commented Nov 4, 2022

Elsewhere I heard some resistance to merging this to master. Since there probably will be a FIP for this work (following filecoin-project/FIPs#401) I'll merge to a new integration branch integration/builtin-api for now.

@anorth anorth changed the base branch from master to integration/builtin-api November 4, 2022 02:17
Copy link
Contributor

@arajasek arajasek left a comment

Choose a reason for hiding this comment

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

I'm in favour of this approach, let's land this.

@arajasek arajasek merged commit 1f9aba6 into integration/builtin-api Nov 6, 2022
@arajasek arajasek deleted the anorth/export-api branch November 6, 2022 14:08
@Stebalien
Copy link
Member

I think off-chain / on-chain isn't the relevant division here

The "account" actor is basically "the user" so there isn't really anything internal about it.

But we can handle this by:

  1. Exporting these methods as suggested by @anorth.
  2. Encouraging users to use the "correct" method numbers, hiding the internal ones.

I toyed briefly with an alternative Enum, but it makes everything else more complicated and in some cases less transparent. Here the churn is confined to the enum values. Of course we can try other concrete approaches too.

I think the main win will be when we have to export, document, and import these APIs. But we can always cross that bridge later.

vyzo pushed a commit that referenced this pull request Dec 7, 2022
* Remove the market actor state mutation pattern (#734)

Fixes #656

* Proof of concept exported API for Account actor (#797)

* Export stable methods for public access (#807)

* Export Datacap Actor methods

* Export Init Actor methods

* Export Market Actor methods

* Export Miner Actor methods

* Export Multisig Actor methods

* Export Verifreg Actor methods

* Address review

* Restrict internal APIs of all actors (#809)

* Exported API method for market actor escrow/locked balance (#812)

* Power actor: Add exported getters for raw power (#810)

* Power actor: Add exported getters for raw power

* FRC-XXXX is FRC-0042

* Power actor: network_raw_power: Return this_epoch_raw_byte_power

* Power actor: miner_raw_power: Return whether above consensus min power

* Power actor: types: serialize one-element structs transparently

* Address review

* Miner actor: Add exported getters for info and monies (#811)

* Miner actor: Add exported getters for info and monies

* Tweak comment

* Miner actor: Replace GetWorker and GetControls with IsControllingAddress

* Miner actor: Add exported GetAvailableBalance

* Miner actor: Add exported GetVestingFunds

* Miner actor: Remove exported monies getters

* Miner actor: types: serialize one-element structs transparently

* Address review

* Address review

* Built-in market API for deal proposal metadata (#818)

* Call exported authenticate method from PSD (#829)


Co-authored-by: zenground0 <ZenGround0@users.noreply.github.com>

* Drop CALLER_TYPES_SIGNABLE and signable caller validation (#821)

* Market actor: Minor improvements to two exported getters (#826)

* Market actor: GetDealTermExported: Return (start_epoch, duration)

* Market actor: Export getter for deal total price

* Exported API for market deal activation state (#819)

* Paych actor: Drop account req, use AuthenticateMessage to verify sigs (#824)

* Paych actor: Drop account req, use AuthenticateMessage to verify sigs

* Address review

* Address review

* Account actor: Deprecate AuthenticateMessage (#856)

* Market actor: Export PublishStorageDeals (#857)

* Miner: Export several more methods (#863)

* Miner: Export ChangeWorkerAddress

* Miner: Export ChangePeerID

* Miner: Export WithdrawBalance

* Miner: Export ChangeMultiaddrs

* Miner: Export ConfirmUpdateWorkerKey

* Miner: Export RepayDebt

* Miner: Export ChangeOwnerAddress

* Miner: Add exported getters for PeerID & multiaddrs

* Miner: Refactor: Rename ConfirmUpdateWorkerKey to ConfirmChangeWorkerAddress

* Power actor: Export methods to CreateMiner and get miner counts (#868)

* Power: Export CreateMiner

* Power Actor: Export MinerCount and MinerConsensusCount

* Update actors/power/src/lib.rs

Co-authored-by: Alex <445306+anorth@users.noreply.github.com>

Co-authored-by: Alex <445306+anorth@users.noreply.github.com>

* Verifreg: Export AddVerifiedClient and GetClaims (#873)

* Verifreg: Rename AddVerifierClientParams to AddVerifiedClientParams

* Verifreg: Export AddVerifiedClient and GetClaims

* Datacap actor: Modify exported methods (#909)

* Datacap: Export Mint and Destroy

* Datacap actor: Deprecate all internal methods

* Datacap actor: Rename BalanceOf to Balance

* Datacap actor: Add Granularity method

* fix: comments on newly exported methods (#910)

Co-authored-by: RK <r.raajey@gmail.com>
Co-authored-by: Alex <445306+anorth@users.noreply.github.com>
Co-authored-by: ZenGround0 <5515260+ZenGround0@users.noreply.github.com>
Co-authored-by: zenground0 <ZenGround0@users.noreply.github.com>
arajasek added a commit that referenced this pull request Jan 12, 2023
* Proof of concept exported API for Account actor (#797)

* Export stable methods for public access (#807)

* Export Datacap Actor methods

* Export Init Actor methods

* Export Market Actor methods

* Export Miner Actor methods

* Export Multisig Actor methods

* Export Verifreg Actor methods

* Address review

* Restrict internal APIs of all actors (#809)

* Exported API method for market actor escrow/locked balance (#812)

* Power actor: Add exported getters for raw power (#810)

* Power actor: Add exported getters for raw power

* FRC-XXXX is FRC-0042

* Power actor: network_raw_power: Return this_epoch_raw_byte_power

* Power actor: miner_raw_power: Return whether above consensus min power

* Power actor: types: serialize one-element structs transparently

* Address review

* Miner actor: Add exported getters for info and monies (#811)

* Miner actor: Add exported getters for info and monies

* Tweak comment

* Miner actor: Replace GetWorker and GetControls with IsControllingAddress

* Miner actor: Add exported GetAvailableBalance

* Miner actor: Add exported GetVestingFunds

* Miner actor: Remove exported monies getters

* Miner actor: types: serialize one-element structs transparently

* Address review

* Address review

* Built-in market API for deal proposal metadata (#818)

* Call exported authenticate method from PSD (#829)


Co-authored-by: zenground0 <ZenGround0@users.noreply.github.com>

* Drop CALLER_TYPES_SIGNABLE and signable caller validation (#821)

* Market actor: Minor improvements to two exported getters (#826)

* Market actor: GetDealTermExported: Return (start_epoch, duration)

* Market actor: Export getter for deal total price

* Exported API for market deal activation state (#819)

* Paych actor: Drop account req, use AuthenticateMessage to verify sigs (#824)

* Paych actor: Drop account req, use AuthenticateMessage to verify sigs

* Address review

* Address review

* Account actor: Deprecate AuthenticateMessage (#856)

* Market actor: Export PublishStorageDeals (#857)

* Miner: Export several more methods (#863)

* Miner: Export ChangeWorkerAddress

* Miner: Export ChangePeerID

* Miner: Export WithdrawBalance

* Miner: Export ChangeMultiaddrs

* Miner: Export ConfirmUpdateWorkerKey

* Miner: Export RepayDebt

* Miner: Export ChangeOwnerAddress

* Miner: Add exported getters for PeerID & multiaddrs

* Miner: Refactor: Rename ConfirmUpdateWorkerKey to ConfirmChangeWorkerAddress

* Power actor: Export methods to CreateMiner and get miner counts (#868)

* Power: Export CreateMiner

* Power Actor: Export MinerCount and MinerConsensusCount

* Update actors/power/src/lib.rs

Co-authored-by: Alex <445306+anorth@users.noreply.github.com>

Co-authored-by: Alex <445306+anorth@users.noreply.github.com>

* Verifreg: Export AddVerifiedClient and GetClaims (#873)

* Verifreg: Rename AddVerifierClientParams to AddVerifiedClientParams

* Verifreg: Export AddVerifiedClient and GetClaims

* Datacap actor: Modify exported methods (#909)

* Datacap: Export Mint and Destroy

* Datacap actor: Deprecate all internal methods

* Datacap actor: Rename BalanceOf to Balance

* Datacap actor: Add Granularity method

* fix: comments on newly exported methods (#910)

* Miner: Export method to GetPendingOwner

* MarketNotifyDeal (#944)


Co-authored-by: zenground0 <ZenGround0@users.noreply.github.com>

* Verifreg: Call AuthenticateMessage instead of validating siggys

* Multisig: Do not export any functionality (#967)

* use const for EX_DEAL_EXPIRED (#954)

* Address review

Co-authored-by: Alex <445306+anorth@users.noreply.github.com>
Co-authored-by: ZenGround0 <5515260+ZenGround0@users.noreply.github.com>
Co-authored-by: zenground0 <ZenGround0@users.noreply.github.com>
Co-authored-by: Shashank <99187193+sudo-shashank@users.noreply.github.com>
shamb0 pushed a commit to shamb0/builtin-actors that referenced this pull request Jan 31, 2023
shamb0 pushed a commit to shamb0/builtin-actors that referenced this pull request Jan 31, 2023
…ecoin-project#1004)

* Proof of concept exported API for Account actor (filecoin-project#797)

* Export stable methods for public access (filecoin-project#807)

* Export Datacap Actor methods

* Export Init Actor methods

* Export Market Actor methods

* Export Miner Actor methods

* Export Multisig Actor methods

* Export Verifreg Actor methods

* Address review

* Restrict internal APIs of all actors (filecoin-project#809)

* Exported API method for market actor escrow/locked balance (filecoin-project#812)

* Power actor: Add exported getters for raw power (filecoin-project#810)

* Power actor: Add exported getters for raw power

* FRC-XXXX is FRC-0042

* Power actor: network_raw_power: Return this_epoch_raw_byte_power

* Power actor: miner_raw_power: Return whether above consensus min power

* Power actor: types: serialize one-element structs transparently

* Address review

* Miner actor: Add exported getters for info and monies (filecoin-project#811)

* Miner actor: Add exported getters for info and monies

* Tweak comment

* Miner actor: Replace GetWorker and GetControls with IsControllingAddress

* Miner actor: Add exported GetAvailableBalance

* Miner actor: Add exported GetVestingFunds

* Miner actor: Remove exported monies getters

* Miner actor: types: serialize one-element structs transparently

* Address review

* Address review

* Built-in market API for deal proposal metadata (filecoin-project#818)

* Call exported authenticate method from PSD (filecoin-project#829)

Co-authored-by: zenground0 <ZenGround0@users.noreply.github.com>

* Drop CALLER_TYPES_SIGNABLE and signable caller validation (filecoin-project#821)

* Market actor: Minor improvements to two exported getters (filecoin-project#826)

* Market actor: GetDealTermExported: Return (start_epoch, duration)

* Market actor: Export getter for deal total price

* Exported API for market deal activation state (filecoin-project#819)

* Paych actor: Drop account req, use AuthenticateMessage to verify sigs (filecoin-project#824)

* Paych actor: Drop account req, use AuthenticateMessage to verify sigs

* Address review

* Address review

* Account actor: Deprecate AuthenticateMessage (filecoin-project#856)

* Market actor: Export PublishStorageDeals (filecoin-project#857)

* Miner: Export several more methods (filecoin-project#863)

* Miner: Export ChangeWorkerAddress

* Miner: Export ChangePeerID

* Miner: Export WithdrawBalance

* Miner: Export ChangeMultiaddrs

* Miner: Export ConfirmUpdateWorkerKey

* Miner: Export RepayDebt

* Miner: Export ChangeOwnerAddress

* Miner: Add exported getters for PeerID & multiaddrs

* Miner: Refactor: Rename ConfirmUpdateWorkerKey to ConfirmChangeWorkerAddress

* Power actor: Export methods to CreateMiner and get miner counts (filecoin-project#868)

* Power: Export CreateMiner

* Power Actor: Export MinerCount and MinerConsensusCount

* Update actors/power/src/lib.rs

Co-authored-by: Alex <445306+anorth@users.noreply.github.com>

Co-authored-by: Alex <445306+anorth@users.noreply.github.com>

* Verifreg: Export AddVerifiedClient and GetClaims (filecoin-project#873)

* Verifreg: Rename AddVerifierClientParams to AddVerifiedClientParams

* Verifreg: Export AddVerifiedClient and GetClaims

* Datacap actor: Modify exported methods (filecoin-project#909)

* Datacap: Export Mint and Destroy

* Datacap actor: Deprecate all internal methods

* Datacap actor: Rename BalanceOf to Balance

* Datacap actor: Add Granularity method

* fix: comments on newly exported methods (filecoin-project#910)

* Miner: Export method to GetPendingOwner

* MarketNotifyDeal (filecoin-project#944)

Co-authored-by: zenground0 <ZenGround0@users.noreply.github.com>

* Verifreg: Call AuthenticateMessage instead of validating siggys

* Multisig: Do not export any functionality (filecoin-project#967)

* use const for EX_DEAL_EXPIRED (filecoin-project#954)

* Address review

Co-authored-by: Alex <445306+anorth@users.noreply.github.com>
Co-authored-by: ZenGround0 <5515260+ZenGround0@users.noreply.github.com>
Co-authored-by: zenground0 <ZenGround0@users.noreply.github.com>
Co-authored-by: Shashank <99187193+sudo-shashank@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