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.

Sign up for GitHub

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(lazer/contracts/evm): Add fees for verification #2163

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

feat(lazer/contracts/evm): Add fees for verification #2163

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fb22a39
Add fees and test
Dec 3, 2024
efad833
Adjust values
Dec 3, 2024
94b6817
Bob calls
Dec 3, 2024
090459c
precommit
Dec 3, 2024
15de6e8
Change fee to 1 wei
Dec 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion lazer/contracts/evm/src/PythLazer.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";

contract PythLazer is OwnableUpgradeable, UUPSUpgradeable {
TrustedSignerInfo[2] public trustedSigners;
Copy link
Collaborator

Choose a reason for hiding this comment

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

The storage is incremental in EVM. I recommend we leave room for more signers here so we can add more signers. A better thing to do is to transition from having a list of signers to a map of addressSigner to expiresAt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just an FYI for this, by replacing it with a mapping, we seem to incur some costs:

  1. We take up semi-random storage slots for hashed items in the mapping instead of a linear sequence of slots. This means collisions become possible, though very unlikely.
  2. Scanning through the items in the mapping are basically impossible, especially considering gas fees. This means migrating the mapping in any way would be pretty difficult since modifying the struct changes the slots taken up by each item, and making a new map would be tricky since it would be hard to transfer the data over.

I would rather increase the size to a comfortable size like 100, rather than change to a map which we would have issues working with. This is based on what I've learned so far, so I could be wrong on some of these things. Please do let me know if something is wrong in my considerations above.

uint256 public verification_fee = 0.00001 ether;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what is an appropriate fee. Changing this value does not impact the tests (Unless the fee is a fractional wei which I don't think is possible?) so we can just change this to whatever we want.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this 1 wei? I thought 1 eth was 10^18 wei


struct TrustedSignerInfo {
address pubkey;
Expand Down Expand Up @@ -62,7 +63,13 @@ contract PythLazer is OwnableUpgradeable, UUPSUpgradeable {

function verifyUpdate(
bytes calldata update
) external view returns (bytes calldata payload, address signer) {
) external payable returns (bytes calldata payload, address signer) {
// Require fee and refund excess
require(msg.value >= verification_fee, "Insufficient fee provided");
if (msg.value > verification_fee) {
payable(msg.sender).transfer(msg.value - verification_fee);
}
keyvankhademi marked this conversation as resolved.
Show resolved Hide resolved

if (update.length < 71) {
revert("input too short");
}
Expand Down
36 changes: 36 additions & 0 deletions lazer/contracts/evm/test/PythLazer.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,40 @@ contract PythLazerTest is Test {
skip(2000);
assert(!pythLazer.isValidSigner(address(2)));
}

function test_verify_with_fee() public {
// Prepare dummy update and signer
address trustedSigner = 0xEfEf56cD66896f6799A90A4e4d512C330c094e44;
vm.prank(address(1));
pythLazer.updateTrustedSigner(trustedSigner, 3000000000000000);
bytes memory update = hex"2a22999a577d3cc0202197939d736bc0dcf71b9dde7b9470e4d16fa8e2120c0787a1c0d744d0c39cc372af4d1ecf2d09e84160ca905f3f597d20e2eec144a446a0459ad600001c93c7d3750006240af373971c01010000000201000000000005f5e100";

uint256 fee = pythLazer.verification_fee();

address alice = makeAddr("alice");
vm.deal(alice, 1 ether);
address bob = makeAddr("bob");
vm.deal(bob, 1 ether);

// Alice provides appropriate fee
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to just bunch these all together. I can also make them separate unit tests too, but I liked the idea of making more than one call on an account and confirming their balance changes as expected.

vm.prank(alice);
pythLazer.verifyUpdate{ value: fee }(update);
assertEq(alice.balance, 1 ether - fee);

// Alice overpays and is refunded
vm.prank(alice);
pythLazer.verifyUpdate{ value: 0.5 ether }(update);
assertEq(alice.balance, 1 ether - fee - fee);

// Bob does not attach a fee
vm.prank(bob);
vm.expectRevert("Insufficient fee provided");
pythLazer.verifyUpdate(update);
assertEq(bob.balance, 1 ether);

// Bob does not attach enough fees
vm.expectRevert("Insufficient fee provided");
pythLazer.verifyUpdate{ value: 1 wei }(update);
assertEq(bob.balance, 1 ether);
}
}
Loading