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

Implements datacap token actor #513

Merged
merged 1 commit into from
Aug 11, 2022
Merged

Conversation

anorth
Copy link
Member

@anorth anorth commented Aug 5, 2022

Implements a standalone datacap fungible token actor, using the Helix fungible token actor library in development for filecoin-project/FIPs#407

Part of filecoin-project/FIPs#313. Not yet integrated with anything. I haven't written tests yet as we're merging to a development branch with priority on ensuring the end-to-end hookup is correct.

This is ready for review, with following caveats:

  • Currently depends on a branch of the token library, pending landing some helpers there
  • Build failing due to some missing symbols from FVM, dunno what's up

Closes #522

@anorth anorth force-pushed the anorth/datacap-token branch 4 times, most recently from 3c4f9d6 to 3582f12 Compare August 9, 2022 02:18
pub fn check_state_invariants<BS: Blockstore>(
_state: &State,
_store: &BS,
) -> (StateSummary, MessageAccumulator) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the token library can provide a generic state invariant check?

@anorth
Copy link
Member Author

anorth commented Aug 10, 2022

@alexytsu I can't request review from you, but would appreciate you taking a look too

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.

If the diff between master and integration were small I would have kept reviewing but the PCD change is now in this PR too. I'm going to update the integration branch first to ease review.

@@ -2,6 +2,9 @@ name: Continuous integration

on:
push:
branches:
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to introduce commits from master to the integration branch like this? I would think we add these commits in separate PRs, or if we can avoid it not at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, oversight. I should have rebased the integration branch and then this first

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.

LGTM overall, I'll be taking look at the Token library internals next

use fvm_shared::bigint::{bigint_ser, BigInt};

#[derive(Clone, Debug, PartialEq, Eq, Serialize_tuple, Deserialize_tuple)]
pub struct AllowanceParams {
Copy link
Contributor

Choose a reason for hiding this comment

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

Have we considered moving the generalized token type parameters into the token library?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, they're not quite ready yet

actors/datacap/tests/datacap_actor_test.rs Outdated Show resolved Hide resolved
pub mod testing;
mod types;

/// TODO:
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we keeping these on the integration branch or trying to remove all TODOs each PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok to keep a few on integration branch

@anorth
Copy link
Member Author

anorth commented Aug 10, 2022

I'll be taking look at the Token library internals next

Still some development going, I reckon wait til next week for that.

@anorth
Copy link
Member Author

anorth commented Aug 11, 2022

Build failure addressed by implementing the token library's Messaging trait in terms of the Runtime. It's a bit messy due to mismatches there, would be easier if the Runtime was moved closer to the real syscall API.

@anorth
Copy link
Member Author

anorth commented Aug 11, 2022

Now pinned to a commit on main of the token library (no crates.io yet)

@anorth anorth merged commit a702969 into decouple-fil+ Aug 11, 2022
@anorth anorth deleted the anorth/datacap-token branch August 11, 2022 01:26
fn actor_id(&self) -> ActorID {
// The Runtime unhelpfully wraps caller in an ID, while the Messaging trait
// is closer to the syscall interface.
self.rt.message().caller().id().unwrap()
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be self.rt.message().receiver().id().unwrap(), it is the ID of this actor itself (used to populate receiver hooks for the from field during minting)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks #519

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.

None yet

3 participants