feat: add support for creating allocations via EVM contract #1970
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
LexLuthr
requested changes
Oct 2, 2024
cmd/boost/direct_deal.go
Outdated
| @@ -93,6 +93,10 @@ var directDealAllocate = &cli.Command{ | |||
| Usage: "automatic yes to prompts; assume 'yes' as answer to all prompts and run non-interactively", | |||
| Aliases: []string{"y", "yes"}, | |||
| }, | |||
| &cli.StringFlag{ | |||
| Name: "evm-client-contract", | |||
| Usage: "address of EVM contract to spend DataCap from", | |||
There was a problem hiding this comment.
We should clarify here that we are expecting F4 address and not Eth address.
cmd/boost/direct_deal.go
Outdated
| return err | ||
| var msgs []*types.Message | ||
| var allocationsAddr address.Address | ||
| evmContract := cctx.String("evm-client-contract") |
There was a problem hiding this comment.
I would suggest checking if the flag is set and if the value if empty then return an error.
cmd/boost/direct_deal.go
Outdated
| @@ -287,7 +307,7 @@ var directDealAllocate = &cli.Command{ | |||
| mcidStr = append(mcidStr, c.String()) | |||
| } | |||
|
|
|||
| log.Infow("submitted data cap allocation message[s]", mcidStr) | |||
| log.Infow("submitted data cap allocation message[s]", "mcidStr", mcidStr) | |||
There was a problem hiding this comment.
Suggested change
| log.Infow("submitted data cap allocation message[s]", "mcidStr", mcidStr) | |
| log.Infow("submitted data cap allocation message[s]", "CID", mcidStr) |
| } | ||
|
|
||
| // Decode return value (cbor -> evm ABI -> math/big Int -> filecoin big Int) | ||
| var decodedReturn abi.CborBytes |
There was a problem hiding this comment.
Check exit code success here before unmarshaling the result
|
Please fix the failing test as well |
|
Code updated as requested in review and bench test is fixed. Manual test results & invoke commands:
|
9 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See #1961 for detailed context.
Summary
This PR adds a new option to
boost allocatecommand:--evm-client-contract. When the option is provided, instead of calling transfer method on datacap actor directly, it invokes transfer method on the provided EVM contract. It's assumed that the contract is an instance of fidlabs Client.sol.Notable changes
As recommended by @LexLuthr , this adds
go-ethereumdependency. This requires replacing go-crypto v0.0.1 with v0.1.0 to make linker happy.Tests
No unit tests were added, as new code relies heavily on API. No integration tests were added, as the feature depends on an external smart contract. If you think unit or integration tests should be added, please provide guidance on how to make this work.
It tested manually using the docker devnet with the following procedure:
boost allocatewith--evm-client-contractoption and verified that it failed'increaseAllowance(address,uint256)'on the Client.sol contract to grant datacap allowance to boost addressboost allocateagain, verified that it succeededlotus filplus list-allocationsto verify that allocation was createdLet me know if detailed instruction for setting up a dev env with the contract is needed.
Known Issues
For me, on docker devnet, it sometimes fails with
03: f06 (method 3726118371) -- allocation expiration 195886 exceeds maximum 195885 (16). The expiration is always 1 higher than the maximum. This is unrelated to the contract change, as this is something I also experience onmainbranch version with normal allocations.