-
Notifications
You must be signed in to change notification settings - Fork 14
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
feat: SDK::send returns ExitCode and Option<IpldBlock> #177
Conversation
064ee7e
to
4c5d9d1
Compare
fvm_actor_utils/src/receiver/mod.rs
Outdated
@@ -132,16 +147,16 @@ impl<T: RecipientData> ReceiverHook<T> { | |||
&TokenAmount::zero(), | |||
)?; | |||
|
|||
match receipt.exit_code { | |||
match ret.0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I think we want to name the return struct. This is going to get confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done :)
2ffd727
to
882fb5c
Compare
882fb5c
to
84801dd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good after deps are clean
frc46_token/src/token/types.rs
Outdated
@@ -118,7 +118,7 @@ pub struct MintReturn { | |||
pub balance: TokenAmount, | |||
/// The new total supply. | |||
pub supply: TokenAmount, | |||
/// (Optional) data returned from receiver hook | |||
/// (Optional) CBOR-encoded data returned from receiver hook |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think anything here forces this to be CBOR, and this comment is likely to rot. cc @Stebalien
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably just switch this over to Option<IpldBlock>
(or actually check the codec).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're both right -- I'm gonna open an issue to make this change. It shouldn't be too hard to do, but I don't wanna increase the scope of this PR.
@anorth I will revert the change to the comment, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fvm_actor_utils/src/messaging.rs
Outdated
@@ -51,6 +50,21 @@ impl From<&MessagingError> for ExitCode { | |||
} | |||
} | |||
|
|||
/// Matches the SDK's Response type | |||
/// We duplicate the type here, because this trait must be implemented |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trait?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I'm referring to the Messaging trait below -- I'll clarify.
frc46_token/src/token/types.rs
Outdated
@@ -118,7 +118,7 @@ pub struct MintReturn { | |||
pub balance: TokenAmount, | |||
/// The new total supply. | |||
pub supply: TokenAmount, | |||
/// (Optional) data returned from receiver hook | |||
/// (Optional) CBOR-encoded data returned from receiver hook |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably just switch this over to Option<IpldBlock>
(or actually check the codec).
Integrates filecoin-project/ref-fvm#1366