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

feat: Implement FIP-0044 #502

Merged
merged 5 commits into from
Aug 31, 2022
Merged

feat: Implement FIP-0044 #502

merged 5 commits into from
Aug 31, 2022

Conversation

arajasek
Copy link
Contributor

@arajasek arajasek commented Jul 22, 2022

This PR is a prototype of filecoin-project/FIPs#413 for illustrative purpsoses.

UPDATE: The FIP draft has since been opened. It uses a different name for the method than this PR, we'll reconcile them based on which is better received.

resolves #533

}
};
let sig = Signature { sig_type, bytes: params.signature };
rt.verify_signature(&sig, &st.address, &params.message).map_err(|e| {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
rt.verify_signature(&sig, &st.address, &params.message).map_err(|e| {
rt.verify_signature(&sig, &address, &params.message).map_err(|e| {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thanks!

actors/account/src/lib.rs Show resolved Hide resolved
@arajasek arajasek force-pushed the asr/authenticate-message branch from 47e4e37 to f979b8a Compare August 3, 2022 14:57
@arajasek arajasek force-pushed the asr/authenticate-message branch from f979b8a to c099a0e Compare August 22, 2022 20:35
@arajasek arajasek changed the title feat: add authenticate message to account actor feat: Implement FIP-0044 Aug 29, 2022
@arajasek arajasek force-pushed the asr/authenticate-message branch from c099a0e to e291350 Compare August 29, 2022 18:51
@arajasek arajasek marked this pull request as ready for review August 29, 2022 18:51
@arajasek
Copy link
Contributor Author

@anorth This is ready for another round of review (maybe even final round).

@arajasek arajasek force-pushed the asr/authenticate-message branch 2 times, most recently from 04f0e63 to e2bbeba Compare August 29, 2022 18:58
Copy link
Member

@anorth anorth left a comment

Choose a reason for hiding this comment

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

Please add a unit test to the account actor. Thanks for the VM test.

Either we don't have any integration tests for deals, or those tests don't make any assertions about the expected invocations from the call trace. Assuming the latter, could you add an ExpectInvocation to at least one of the PSD integration tests please?

@arajasek arajasek force-pushed the asr/authenticate-message branch 5 times, most recently from 849008e to bbdb4e4 Compare August 30, 2022 17:14
@arajasek
Copy link
Contributor Author

@anorth Done -- this now has both unit and integ test coverage of both the account and market changes.

@@ -20,6 +20,7 @@ num-traits = "0.2.14"
num-derive = "0.3.3"
fvm_ipld_blockstore = "0.1.1"
fvm_ipld_encoding = "0.2.2"
anyhow = "1.0.56"
Copy link
Member

Choose a reason for hiding this comment

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

Noooo, we're trying to kill this. It looks like it's only come in to a test, which I could believe is necessary due to existing use of anyhow. But if it's easy to avoid, I'd prefer that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, unfortunately it'd be a bit much to change the runtime test_utils here.

actors/account/src/types.rs Outdated Show resolved Hide resolved
test_vm/tests/publish_deals_test.rs Outdated Show resolved Hide resolved
test_vm/tests/publish_deals_test.rs Outdated Show resolved Hide resolved
@arajasek arajasek force-pushed the asr/authenticate-message branch from bbdb4e4 to 6a3dd21 Compare August 31, 2022 14:34
@arajasek arajasek enabled auto-merge (squash) August 31, 2022 14:34
@arajasek arajasek merged commit f5855ac into master Aug 31, 2022
@arajasek arajasek deleted the asr/authenticate-message branch August 31, 2022 14:56
@jennijuju
Copy link
Member

@arajasek this closes #559 ?

@arajasek
Copy link
Contributor Author

arajasek commented Oct 3, 2022

@jennijuju Yes

ZenGround0 pushed a commit that referenced this pull request Oct 26, 2022
* feat: add authenticate message to account actor

* hook up market actor to call new authenticate_message method

* fixup and add tests
ZenGround0 pushed a commit that referenced this pull request Oct 27, 2022
* feat: Implement FIP-0044 (#502)
* test: modify test_vm's verify_signature behaviour (#511)
* fmt / rebase fixes
shamb0 pushed a commit to shamb0/builtin-actors that referenced this pull request Jan 31, 2023
* feat: add authenticate message to account actor

* hook up market actor to call new authenticate_message method

* fixup and add tests
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.

Implement standard message authentication method on Account actor
3 participants