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

Mt and aa2 #91

Merged
merged 6 commits into from
Dec 15, 2023
Merged

Mt and aa2 #91

merged 6 commits into from
Dec 15, 2023

Conversation

sander2
Copy link
Contributor

@sander2 sander2 commented Nov 16, 2023

No description provided.

Copy link

vercel bot commented Nov 16, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
bob ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 12, 2023 1:35pm

Copy link
Contributor

@peterslany peterslany left a comment

Choose a reason for hiding this comment

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

Changes look good, just a couple of non-blocking comments.

Comment on lines +102 to +103
# Dummy oracle: 0x31b36BB047f6D5e3B49E95c4c99Cce4591e82E3f
# Dummy oracle: 0x6669d0C53fCf30c00F5AbE5a32cFa2EaD2bc2d5a
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: Might be useful to add which asset price those oracle report.

@@ -1,5 +1,5 @@
// SPDX-License-Identifier: MIT
pragma solidity 0.8.18;
pragma solidity ^0.8.13;
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking whether we should lock the compiler version or not, I would suggest to do so if we are done with the development, as per the best practices: https://consensys.github.io/smart-contract-best-practices/development-recommendations/solidity-specific/locking-pragmas/

Copy link
Contributor

Choose a reason for hiding this comment

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

Aside from the security implications, it can also help us with verifying the deployed bytecode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok yea will keep that in mind, but it was giving me compilation errors so I changed it. We'll see if we can lock version prior to mainnet launch

Comment on lines +33 to +39
function _msgSender() internal view override(Context, ERC2771Recipient) returns (address sender) {
sender = ERC2771Recipient._msgSender();
}

function _msgData() internal view override(Context, ERC2771Recipient) returns (bytes calldata) {
return ERC2771Recipient._msgData();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it needed to override these functions when they return the same value as if they weren't overrode?

@sander2 sander2 merged commit 5ef0162 into master Dec 15, 2023
3 checks passed
@sander2 sander2 deleted the mt-and-aa2 branch December 15, 2023 10:03
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