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

Foundry imporvements #1011

Merged
merged 16 commits into from
Dec 16, 2024
Merged

Foundry imporvements #1011

merged 16 commits into from
Dec 16, 2024

Conversation

technophile-04
Copy link
Collaborator

@technophile-04 technophile-04 commented Dec 10, 2024

To test:

Follow the REAMDE and also please feel free to suggest tweaks to README! We should update our docs too next once this README is kind of finalized.

Changes done:

  • Remember deployments
    • Before: if you first deployed YourContract.sol to sepolia and then try to deploy MyContract.sol commenting out the YourContract.sol deployment it was overriding all the previous deployment and only MyContract abi and address was present.
    • Now: It makes sure it also have previous deployments in deployedContracts.ts.
    • NOTE: If you didn't comment YourContract.sol deployment it would deploy new YourContract, in hardaht-deploy it wouldn't have deployed it again. But here it does.
  • Feature to run a particular deploy script.
    • In hardhat-deploy we could do it with --tags
    • Here now you could pass --file <file_name> to yarn deploy and it would run that only.
  • Added some comments in Deploy scripts so that people get better gist of how to use.
  • Updated README with keystore info nicely.

@technophile-04
Copy link
Collaborator Author

Just requesting review from Pablo for basic test and readme suggestions. But also @carletex and @rin-st please chime for README reivew as well code 🙌

Copy link
Collaborator

@Pabl0cks Pabl0cks left a comment

Choose a reason for hiding this comment

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

GJ!! And thanks for tackling this @technophile-04 !! Looks like a tough one.

I've been testing the workflow of keystore + deploys for a bit, have taken these notes to check the right behaviour with you:

  • Should we update the contract address when you redeploy without abi changes (just logic)? We're redeploying the contract, but not updating the deployedContracts.ts contract address.

  • If I try to yarn generate but I already have scaffold-eth-custom keystore, I get this error. Should we give more specific error saying you should delete it if you want to regenerate, or let them set names for the keystore to create a different one?

    yarn generate
    Error: 
    Keystore file already exists at /home/pablo/.foundry/keystores/scaffold-eth-custom
    make: *** [Makefile:59: account-generate] Error 1
    
  • Can ignore, could not reproduce (will try again), think is a Cursor terminal bug. It deployed to anvil instead of throwing me some network error:
    image

  • Retyping the command, got it right I think, but the error looks generic, maybe we could throw some kind of "network does not exist..." error in this case?
    image

  • If you try to deploy with the default keystore to prod, you get prompted for password, but you don't really know. Should we change the behaviour?

  • If you use custom keystore for local network, you get prompted for your password, the readme looks like you wouldn't, maybe need to tweak the No password needed for deployment adding "if you use default keystore".

I'll do a bit more testing and check the readme more carefully later 🙌

@technophile-04
Copy link
Collaborator Author

Should we update the contract address when you redeploy without abi changes (just logic)? We're redeploying the contract, but not updating the deployedContracts.ts contract address.

Thanks! yeah actually it was a bug fixed it

Can ignore, could not reproduce (will try again), think is a Cursor terminal bug. It deployed to anvil instead of throwing me some network error:

Retyping the command, got it right I think, but the error looks generic, maybe we could throw some kind of "network does not exist..." error in this case?

Can't reproduce the first one but now we catch it early and give this error:

image

If you try to deploy with the default keystore to prod, you get prompted for password, but you don't really know. Should we change the behaviour?

Now we catch it early and show it. Please feel free to suggest rewording or push it directly 🙌

image

If you use custom keystore for local network, you get prompted for your password, the readme looks like you wouldn't, maybe need to tweak the No password needed for deployment adding "if you use default keystore".

Ohh yes just tweaked the REAME a bit at e37e4a6.

Also now we give warning that u are using custom account when using local network.

Screenshot 2024-12-13 at 10 54 02 AM

Probably in next PR maybe in localhost we always use scaffold-eth-default but I see some usefulness in using custom account for local development.

If I try to yarn generate but I already have scaffold-eth-custom keystore, I get this error. Should we give more specific error saying you should delete it if you want to regenerate, or let them set names for the keystore to create a different one?

Yeah actually that's foundry error :(. Also actually we are planning to improve keystore flow in #972 so maybe we can improve some stuff there 🙌

Tysm Pablo for detailed tests!

@Pabl0cks
Copy link
Collaborator

Thanks for the quick fixes Shiv!

It's working nicely now, except for the yarn deploy command, which is not updating contract addresses (when you deploy a specific deploy file works great). Tried having either 1 or all the contracts deployments into the Deploy.s.sol file.

Can't reproduce the first one but now we catch it early and give this error:

Can totally ignore it, I just saw it's a cursor terminal bug, I was watching the right command but it understood it wrongly (happens when you modify something in the middle of an already created command):

image

@technophile-04
Copy link
Collaborator Author

It's working nicely now, except for the yarn deploy command, which is not updating contract addresses (when you deploy a specific deploy file works great). Tried having either 1 or all the contracts deployments into the Deploy.s.sol file

Strange can you give me an example how you are doing this? Like I tried creating YourContract.sol and MyContract.sol and created corresponding DeployYourContract.s.sol and DeployMyContract.s.sol and imported them Deploy.s.sol and running yarn deploy give me this which does seem to give the latest contract address:

Screen.Recording.2024-12-13.at.7.40.48.PM.mov

also tried yarn deploy --file DeployMyContract.s.sol and it seems to work.

@rin-st
Copy link
Member

rin-st commented Dec 13, 2024

Great changes! Thank you @technophile-04 and @Pabl0cks !

Strange can you give me an example how you are doing this? Like I tried creating YourContract.sol and MyContract.sol and created corresponding DeployYourContract.s.sol and DeployMyContract.s.sol and imported them Deploy.s.sol and running yarn deploy give me this which does seem to give the latest contract address:

It works for me the same way

@Pabl0cks
Copy link
Collaborator

Strange can you give me an example how you are doing this? Like I tried creating YourContract.sol and MyContract.sol and created corresponding DeployYourContract.s.sol and DeployMyContract.s.sol and imported them Deploy.s.sol and running yarn deploy give me this which does seem to give the latest contract address:

I got this in my Deploy.s.sol file:

pragma solidity ^0.8.19;

import "./DeployHelpers.s.sol";
import {DeployYourContract} from "./DeployYourContract.s.sol";
import {DeployMyContract} from "./DeployMyContract.s.sol";
/**
 * @notice Main deployment script for all contracts
 * @dev Run this when you want to deploy multiple contracts at once
 *
 * Example: yarn deploy # runs this script(without`--file` flag)
 */
contract DeployScript is ScaffoldETHDeploy {
    function run() external {
        // Deploys all your contracts sequentially
        // Add new deployments here when needed

        // Deploy YourContract
        DeployYourContract yourContract = new DeployYourContract();
        yourContract.run();

        // Deploy another contract
        DeployMyContract myContract = new DeployMyContract();
        myContract.run();
    }
}

It looks like the contract has been deployed correctly, but not getting updated in the deployedContracts.ts file, I still see the old CA (same that was happening before with the single deploy fiels):

image

@technophile-04
Copy link
Collaborator Author

It looks like the contract has been deployed correctly, but not getting updated in the deployedContracts.ts file, I still see the old CA (same that was happening before with the single deploy fiels):

Umm can you delete all this folders and try again? :

Screenshot 2024-12-14 at 10 06 30 AM

@Pabl0cks
Copy link
Collaborator

Umm can you delete all this folders and try again? :

It worked after deletion, but found the pattern to reproduce my problem. Not sure if it will be a WSL related thing. Steps:

  • yarn deploy after cache deletion, works fine
  • Then you yarn deploy --file DeployYourContract.s.sol => It updates YourContract nicely
  • Then you yarn deploy again, realized only MyContract was getting updated, YourContract got stuck with the individual deployment CA
  • If you deploy MyContract individually, it stays stuck too, so none of my contracts would update with yarn deploy

@technophile-04
Copy link
Collaborator Author

TYSM Pablo for the reproduction steps 🙏 it was actually indeed a bug and generateTsAbis.js script.

Fixed at 9ced131 can you please try again?

Here are some test files incase it helps:

MyContract.sol
//SPDX-License-Identifier: MIT
pragma solidity >=0.8.0 <0.9.0;

// Useful for debugging. Remove when deploying to a live network.
import "forge-std/console.sol";

// Use openzeppelin to inherit battle-tested implementations (ERC20, ERC721, etc)
// import "@openzeppelin/contracts/access/Ownable.sol";

/**
 * A smart contract that allows changing a state variable of the contract and tracking the changes
 * It also allows the owner to withdraw the Ether from the contract
 * @author BuidlGuidl
 */
contract MyContract {
    // State Variables
    address public immutable owner;
    string public greeting = "Building Unstoppable Apps!!!";
    bool public premium = false;
    uint256 public totalCounter = 0;
    mapping(address => uint256) public userGreetingCounter;

    // Events: a way to emit log statements from smart contract that can be listened to by external parties
    event GreetingChange(address indexed greetingSetter, string newGreeting, bool premium, uint256 value);

    // Constructor: Called once on contract deployment
    // Check packages/foundry/deploy/Deploy.s.sol
    constructor(address _owner) {
        owner = _owner;
    }

    // Modifier: used to define a set of rules that must be met before or after a function is executed
    // Check the withdraw() function
    modifier isOwner() {
        // msg.sender: predefined variable that represents address of the account that called the current function
        require(msg.sender == owner, "Not the Owner");
        _;
    }

    /**
     * Function that allows anyone to change the state variable "greeting" of the contract and increase the counters
     *
     * @param _newGreeting (string memory) - new greeting to save on the contract
     */
    function setGreeting(string memory _newGreeting) public payable {
        // Print data to the anvil chain console. Remove when deploying to a live network.

        console.logString("Setting new greeting");
        console.logString(_newGreeting);

        greeting = _newGreeting;
        totalCounter += 1;
        userGreetingCounter[msg.sender] += 1;

        // msg.value: built-in global variable that represents the amount of ether sent with the transaction
        if (msg.value > 0) {
            premium = true;
        } else {
            premium = false;
        }

        // emit: keyword used to trigger an event
        emit GreetingChange(msg.sender, _newGreeting, msg.value > 0, msg.value);
    }

    /**
     * Function that allows the owner to withdraw all the Ether in the contract
     * The function can only be called by the owner of the contract as defined by the isOwner modifier
     */
    function withdraw() public isOwner {
        (bool success,) = owner.call{ value: address(this).balance }("");
        require(success, "Failed to send Ether");
    }

    /**
     * Function that allows the contract to receive ETH
     */
    receive() external payable { }
}
DeployMyContract.s.sol
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;

import "./DeployHelpers.s.sol";
import "../contracts/MyContract.sol";

/**
 * @notice Deploy script for YourContract contract
 * @dev Inherits ScaffoldETHDeploy which:
 *      - Includes forge-std/Script.sol for deployment
 *      - Includes ScaffoldEthDeployerRunner modifier
 *      - Provides `deployer` variable
 * Example:
 * yarn deploy --file DeployYourContract.s.sol  # local anvil chain
 * yarn deploy --file DeployYourContract.s.sol --network optimism # live network (requires keystore)
 */
contract DeployMyContract is ScaffoldETHDeploy {
    /**
     * @dev Deployer setup based on `ETH_KEYSTORE_ACCOUNT` in `.env`:
     *      - "scaffold-eth-default": Uses Anvil's account #9 (0xa0Ee7A142d267C1f36714E4a8F75612F20a79720), no password prompt
     *      - "scaffold-eth-custom": requires password used while creating keystore
     *
     * Note: Must use ScaffoldEthDeployerRunner modifier to:
     *      - Setup correct `deployer` account and fund it
     *      - Export contract addresses & ABIs to `nextjs` packages
     */
    function run() external ScaffoldEthDeployerRunner {
        new MyContract(deployer);
    }
}
Updated Deploy.s.sol
//SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;

import "./DeployHelpers.s.sol";
import { DeployYourContract } from "./DeployYourContract.s.sol";
import { DeployMyContract } from "./DeployMyContract.s.sol";

/**
 * @notice Main deployment script for all contracts
 * @dev Run this when you want to deploy multiple contracts at once
 *
 * Example: yarn deploy # runs this script(without`--file` flag)
 */
contract DeployScript is ScaffoldETHDeploy {
    function run() external {
        // Deploys all your contracts sequentially
        // Add new deployments here when needed

        // Deploy YourContract
        DeployYourContract yourContract = new DeployYourContract();
        yourContract.run();

        // Deploy another contract
        DeployMyContract myContract = new DeployMyContract();
        myContract.run();
    }
}

Copy link
Collaborator

@Pabl0cks Pabl0cks left a comment

Choose a reason for hiding this comment

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

Now is working GREAT to me! TY Shiv 🙌🙌

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