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

Transfer-and-Call, Transfer-Multi-and-Call, Get-Call-Data and other enhancements #12

Merged
merged 9 commits into from
Aug 30, 2024

Conversation

IaroslavMazur
Copy link
Member

@IaroslavMazur IaroslavMazur commented Jun 17, 2024

feat: implement contract to test transfer-and-call functionality
feat: infra for testing transfer-multi-and-call
feat: Get Call Data functionality

fix: send MNTs to a single address, instead of many
fix: make the MNTs sender implicit (i.e. the msg.sender)
fix: transfer and call
fix: make NativeTokens::balanceOf() callable by both EOAs and Smart Contracts

test: NaiveTokenTransferrerMock

refactor: make the token transfer sender (i.e. address(this)) implicit
refactor: restrict the burning ability to just the token holders
refactor: change the signature of INativeTokens::burn()
refactor: change the type of sub id and token id to uint256
refactor: remove the unused code

chore: format the code in the repo
chore: work around the forge fmt <-> bun solhint max-line conflict

@IaroslavMazur IaroslavMazur requested a review from PaulRBerg June 17, 2024 19:49
@IaroslavMazur IaroslavMazur force-pushed the iaro/mint-and-burn-limitation branch 3 times, most recently from 18f428f to ec9685c Compare June 17, 2024 20:22
@IaroslavMazur
Copy link
Member Author

@PaulRBerg, curiously enough, there seems to be an issue with the "max line length" parameter of forge's formatter, in which the maximum acceptable length for a code line is considered to be N+1, instead of the N you've submitted as line_length in the [fmt] section of foundry.toml 🤔

Because of the above, the "same" max line limit for forge's and solhint's formatter was, actually, off by one - and was generating an error during the execution of the lint job in CI.

I've worked around this by configuring the forge fmt's line_length to 119.

@PaulRBerg
Copy link
Member

@IaroslavMazur that's a known issue with the Forge formatter:

foundry-rs/foundry#4450

Rather than lowering the formatter's line length to 119, we should instead increase Solhint's max-line-length, just like we did in the Lockup repo:

https://github.com/sablier-labs/v2-core/blob/8a3121013e2b199dcbac0a83db1c079843f012c3/.solhint.json#L12

@IaroslavMazur IaroslavMazur force-pushed the iaro/mint-and-burn-limitation branch from ec9685c to 8c7d0cc Compare June 18, 2024 08:39
@IaroslavMazur
Copy link
Member Author

@IaroslavMazur that's a known issue with the Forge formatter:
foundry-rs/foundry#4450

Oh, wow... I did try looking for this issue reported in their repo, and did notice the issue above, but haven't looked deeper into it, because thought that it refers to smth else. 😅

Well done reporting the problem!


Rather than lowering the formatter's line length to 119, we should instead increase Solhint's max-line-length, just like we did in the Lockup repo:

Thanks for the idea! Just pushed the new changes.

Given how in Solhint, the max line length is a hard requirement, it does make sense to configure it there (i.e. configuring it in Foundry's formatter would only give us unpredictable results).

@IaroslavMazur IaroslavMazur force-pushed the iaro/mint-and-burn-limitation branch from 8c7d0cc to 00df50a Compare June 18, 2024 16:50
@IaroslavMazur IaroslavMazur force-pushed the iaro/mint-and-burn-limitation branch from 00df50a to e49ac92 Compare June 27, 2024 13:08
@IaroslavMazur
Copy link
Member Author

TIL: it is perfectly fine for Solidity libraries to have internal functions.

To generate the "callable" bytecode for the NativeTokens lib in our SabVM tests, instead of making the lib functions external and deploying the lib "independently" (i.e. to a separate address), we should, rather, use the bytecode associated with the contract(-s) (i.e. SRF20Mock, in our case) that end up using those internal lib functions (and, therefore, contain the definitions of those functions inline).

Pushed the updates to this PR.

cc @PaulRBerg

@IaroslavMazur IaroslavMazur force-pushed the iaro/mint-and-burn-limitation branch 2 times, most recently from 73b9f9a to df2a78a Compare June 28, 2024 15:36
@IaroslavMazur
Copy link
Member Author

I've realized that given

  • the current architecture of the NativeTokens library (that delegateCalls into our Rust Precompile),
  • as well as the way its functionality is being accessed by the contracts implementing the SRF20 standard (via the use NativeTokens for address instruction),

the NativeTokens library is not the right place for checking whether the Native Tokens are, indeed, being minted/burned by a contract (and not an EOA).

This is because in the current design, the EOA calling the SRF20 contract remains the msg.sender (and so, we can't base our verification on it).

The right place for the verification is, therefore, our Rust Precompile. Its ContextPrecompile type hasn't natively supported accessing the "msg sender", but I've brought this info into its context.

Pushed the updates to this PR.

cc @PaulRBerg

@IaroslavMazur IaroslavMazur force-pushed the iaro/mint-and-burn-limitation branch 2 times, most recently from 249a174 to c4ce18d Compare July 1, 2024 14:32
@IaroslavMazur IaroslavMazur changed the title feat: limit minting & burning just to contracts fix: NativeTokens::balanceOf() now callable by EOAs Jul 1, 2024
@IaroslavMazur IaroslavMazur force-pushed the iaro/mint-and-burn-limitation branch 2 times, most recently from 0acf824 to 520ee94 Compare July 3, 2024 03:08
fix: send MNTs to a single address, instead of many;
fix: make the MNTs sender implicit (i.e. the msg.sender).

test: NaiveTokenTransferrerMock

refactor: make the token transfer sender (i.e. `address(this)`) implicit
refactor: restrict the burning ability to just the token holders;
refactor: change the signature of INativeTokens::burn();
refactor: change the type of sub id and token id to uint256;
refactor: remove the unused code;

chore: format the code in the repo;
chore: work around the forge fmt <-> bun solhint max-line conflict.
@IaroslavMazur IaroslavMazur force-pushed the iaro/mint-and-burn-limitation branch 2 times, most recently from 060666f to 8d35938 Compare July 7, 2024 09:43
@IaroslavMazur IaroslavMazur force-pushed the iaro/mint-and-burn-limitation branch from 8d35938 to 348ac36 Compare July 26, 2024 12:48
@IaroslavMazur IaroslavMazur force-pushed the iaro/mint-and-burn-limitation branch from 463e5ef to 4a34413 Compare August 14, 2024 14:39
fix: make NativeTokens::balanceOf() callable by both EOAs and Smart
	 Contracts
Having it also callable by EOAs (i.e. with the `public` modifier) leads
to the to-be-linked dependencies/references inserted into the bytecode
of the contracts using the Native Tokens library. The latter was not
compatible with the testing framework/methodology we use in the SabVM
repo.
@IaroslavMazur IaroslavMazur force-pushed the iaro/mint-and-burn-limitation branch from 3a1f512 to c02dd5c Compare August 22, 2024 12:13
@IaroslavMazur IaroslavMazur force-pushed the iaro/mint-and-burn-limitation branch from 8ed1716 to 79b0b53 Compare August 23, 2024 15:00
@IaroslavMazur IaroslavMazur changed the title fix: NativeTokens::balanceOf() now callable by EOAs Transfer-and-Call, Transfer-Multi-and-Call, Get-Call-Data and other enhancements Aug 23, 2024
@IaroslavMazur IaroslavMazur force-pushed the iaro/mint-and-burn-limitation branch from 79b0b53 to 3000197 Compare August 23, 2024 15:47
@IaroslavMazur IaroslavMazur force-pushed the iaro/mint-and-burn-limitation branch from 3000197 to a6598cb Compare August 23, 2024 15:49
@IaroslavMazur
Copy link
Member Author

Ready to be reviewed by you, @PaulRBerg & @smol-ninja 🙌

@PaulRBerg
Copy link
Member

Great work @IaroslavMazur. I am glad that we have been able to pull this off.

Given that we've decided to turn this work into an EIP, I will trust that your work is good enough as a PoC, and skip the review.

Freel tree to merge this.

@IaroslavMazur
Copy link
Member Author

I am glad that we have been able to pull this off.

Oh, you're not alone in this feeling! 😃😌🥳

@smol-ninja, would you like to take a look - or do I have the green light to merge the PR? 👀

@smol-ninja
Copy link
Member

I will have a look at it soon.

@IaroslavMazur
Copy link
Member Author

I will have a look at it soon.

Awesome, thank you!

@IaroslavMazur IaroslavMazur force-pushed the iaro/mint-and-burn-limitation branch from f76018d to 4c891cd Compare August 28, 2024 15:44
@IaroslavMazur IaroslavMazur force-pushed the iaro/mint-and-burn-limitation branch from 4c891cd to 1d7539c Compare August 28, 2024 15:47
Copy link
Member

@smol-ninja smol-ninja left a comment

Choose a reason for hiding this comment

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

LGTM

@IaroslavMazur IaroslavMazur merged commit a5cc265 into main Aug 30, 2024
2 checks passed
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.

3 participants