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

Add types for one-value params and returns #1242

Merged
merged 11 commits into from
Mar 13, 2023
Merged

Conversation

alexytsu
Copy link
Contributor

@alexytsu alexytsu commented Mar 9, 2023

@codecov-commenter
Copy link

codecov-commenter commented Mar 9, 2023

Codecov Report

Merging #1242 (bab3a04) into master (9cb8a6b) will decrease coverage by 0.02%.
The diff coverage is 80.37%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1242      +/-   ##
==========================================
- Coverage   90.97%   90.96%   -0.02%     
==========================================
  Files         133      133              
  Lines       26633    26697      +64     
==========================================
+ Hits        24230    24284      +54     
- Misses       2403     2413      +10     
Impacted Files Coverage Δ
actors/evm/src/types.rs 62.50% <40.00%> (-37.50%) ⬇️
actors/datacap/src/lib.rs 77.03% <40.90%> (-2.26%) ⬇️
actors/datacap/src/types.rs 69.23% <60.00%> (-30.77%) ⬇️
actors/evm/src/lib.rs 93.12% <96.55%> (+0.41%) ⬆️
actors/account/src/lib.rs 90.00% <100.00%> (+0.52%) ⬆️
actors/account/src/types.rs 100.00% <100.00%> (ø)
actors/market/src/lib.rs 91.13% <100.00%> (ø)
actors/market/src/types.rs 96.77% <100.00%> (+0.22%) ⬆️
actors/miner/src/lib.rs 83.24% <100.00%> (+0.01%) ⬆️
actors/miner/src/types.rs 95.89% <100.00%> (+0.05%) ⬆️
... and 6 more

... and 3 files with indirect coverage changes

@alexytsu alexytsu requested review from arajasek and anorth March 9, 2023 05:10
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.

Great. I would like someone else to check the EVM actors, but thank you.

As an aside, it could be nice to implement a constructor-generating macro like https://jeltef.github.io/derive_more/derive_more/constructor.html so we could token.balance_of(&params.address).map(BalanceReturn::new). But don't hold up this PR for it, and I would do something simple here rather than add a dependency.


#[derive(Clone, Debug, PartialEq, Eq, Serialize_tuple, Deserialize_tuple)]
#[serde(transparent)]
pub struct NameReturn {
Copy link
Member

Choose a reason for hiding this comment

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

Could you add an issue in https://github.com/helix-onchain/filecoin to move these types there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actors/power/src/lib.rs Show resolved Hide resolved
actors/reward/src/types.rs Show resolved Hide resolved
@anorth
Copy link
Member

anorth commented Mar 9, 2023

@Stebalien could you review just the EVM actors for a second set of eyes please?

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

one fix but the EVM logic otherwise looks correct.

actors/evm/src/types.rs Show resolved Hide resolved
@anorth
Copy link
Member

anorth commented Mar 13, 2023

@alexytsu ready for merge? Please do so after rebasing.

@alexytsu alexytsu force-pushed the alex/api-consistency-817 branch from bab3a04 to 86891dc Compare March 13, 2023 22:42
@alexytsu alexytsu enabled auto-merge (squash) March 13, 2023 22:42
@alexytsu alexytsu merged commit 373ada5 into master Mar 13, 2023
@alexytsu alexytsu deleted the alex/api-consistency-817 branch March 13, 2023 23:08
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