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: plugin and chain log #38

Merged
merged 25 commits into from
May 11, 2023
Merged

feat: plugin and chain log #38

merged 25 commits into from
May 11, 2023

Conversation

PaulRBerg
Copy link
Member

@PaulRBerg PaulRBerg commented May 8, 2023

Implements #19, #36, and #37.

@andreivladbrg, you can disregard the fact that the ChainLog contract is incomplete and untested. The goal was to implement an MVP to show case my rationale.

I managed to fully test the ChainLog contract.

Copy link
Member

@andreivladbrg andreivladbrg left a comment

Choose a reason for hiding this comment

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

Let's not ignore the lockup parameter in the onStreamCanceled function:

Code Snippet

    function onStreamCanceled(
        ISablierV2Lockup lockup,
        uint256 streamId,
        address, /* recipient */
        uint128 senderAmount,
        uint128 /* recipientAmount */
    )
        external
        noStandardCall
    {
        // Checks: the caller is Sablier.
        if (!chainLog.isListed(address(lockup))) {
            revert Errors.SablierV2ProxyPlugin_CallerNotSablier(address(lockup));
        }

        // This invariant should always hold but it's better to be safe than sorry.
        address streamSender = lockup.getSender(streamId);
        assert(streamSender == address(this));

        // Effects: forward the refunded assets to the proxy owner.
        IERC20 asset = lockup.getAsset(streamId);
        asset.safeTransfer({ to: owner, value: senderAmount });
    }

Very nice idea with the new abstract contract, but I think it would be better if we renamed NoStandardCall to something like OnlyDelegateCall. The term "Standard" doesn't provide enough clarity since many things can be considered standard, let's be more precise.

No tests for SablierV2ChainLog.

src/SablierV2ProxyPlugin.sol Show resolved Hide resolved
src/SablierV2ChainLog.sol Outdated Show resolved Hide resolved
src/SablierV2ChainLog.sol Show resolved Hide resolved
src/SablierV2ChainLog.sol Show resolved Hide resolved
test/unit/plugin/on-stream-canceled/onStreamCanceled.tree Outdated Show resolved Hide resolved
test/Base.t.sol Outdated Show resolved Hide resolved
test/Base.t.sol Outdated Show resolved Hide resolved
test/Base.t.sol Outdated Show resolved Hide resolved
test/Base.t.sol Outdated Show resolved Hide resolved
src/libraries/Errors.sol Outdated Show resolved Hide resolved
@PaulRBerg
Copy link
Member Author

PaulRBerg commented May 8, 2023

Let's not ignore the lockup parameter in the onStreamCanceled function:

Actually, we will ignore it. See sablier-labs/v2-core#470.

Very nice idea with the new abstract contract, but I think it would be better if we renamed NoStandardCall to something like OnlyDelegateCall

Thanks. I agree with your suggestion.

No tests for SablierV2ChainLog.

See my top comment.

@PaulRBerg
Copy link
Member Author

I have made a few improvements, and also written tests for SablierV2ChainLog.

@andreivladbrg, can you please review when you have a moment.

@PaulRBerg PaulRBerg force-pushed the feat/plugin-and-chain-log branch 2 times, most recently from 9db4880 to 82385cd Compare May 9, 2023 08:32
Copy link
Member

@andreivladbrg andreivladbrg left a comment

Choose a reason for hiding this comment

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

test/utils/Events.sol Outdated Show resolved Hide resolved
test/unit/chain-log/unlist/unlist.t.sol Outdated Show resolved Hide resolved
@PaulRBerg
Copy link
Member Author

I think there is a mistake, either here:

Yep, you're right. ISablierV2ProxyPlugin has three inherited components. I will fix this.

@PaulRBerg PaulRBerg force-pushed the main branch 3 times, most recently from d358b21 to d648d01 Compare May 9, 2023 21:13
@PaulRBerg
Copy link
Member Author

@andreivladbrg - I have addressed all of your feedback above. Is this PR good to merge now?

@PaulRBerg
Copy link
Member Author

Good to merge now, @andreivladbrg?

andreivladbrg and others added 9 commits May 10, 2023 19:00
test: add proxyHelpers testing contract
test: add installPlugin helper function
chore: say target instead of proxyTarget
test: onStreamCanceled function
test: methodList function
test: add assertEq function for bytes4[]
test: remove unnecessary assertEq functions
docs: improve wording in natspec
chore: various improvements in comments
test: change the header for the target functions in Base_Test
test: cast to uint256 in assertEq function for the Status
refactor: rename header separators
test: add more explanatory comments
test: simplify variables in "test_MethodList"
test: use periphery contract for checking function signature
PaulRBerg and others added 15 commits May 10, 2023 19:00
build: bump prb-proxy
refactor: load "owner" from storage directly
chore: add explanatory comments
docs: improve writing
feat: add "InvalidCall" error
test: add new helper for "createWithRange"
test: test direct call to "onStreamCanceled"
test: test invalid calls to "onStreamCanceled"
test: improve writing in comments
docs: improve writing in comments
feat: add "NoStandardCall"
feat: add chain log to "SablierV2ProxyPlugin"
feat: add interface for "SablierV2ProxyPlugin"
feat: add script for deploying "SablierV2ChainLog"
feat: check chain log in "onStreamCanceled"
refactor: add new errors
refactor: delete "InvalidCall" error
refactor: disallow standard calls in "onStreamCanceled"
test: add "eve" user
test: add "listContracts" helper in "Base_Test"
test: various small improvements
test: update tests for "onStreamCanceled"
docs: improve writing in comments
refactor: improve error names
refactor: name mapping params
test: improve state trees
test: use interfaces to cast deployments
ci: add deployment scripts
docs: add ascii art
test: add "Events" abstract contract
test: delete "listContracts" helper
test: improve wording in state trees
test: list contracts in "onStreamCanceled"
test: test "listAddress"
refactor: rename "listAddress" to list"
chore: fix number of inherited components in comments
@PaulRBerg PaulRBerg merged commit 0e851a6 into main May 11, 2023
@PaulRBerg PaulRBerg deleted the feat/plugin-and-chain-log branch May 11, 2023 09:19
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.

2 participants