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

remove Address.sol, replace isContract with address(...).code.length > 0 #817

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

Conversation

eddiehsu66
Copy link

What type of PR is this (这是什么类型的PR)

  • Typo(勘误)
  • BUG(程序错误)
  • Improvement(提升)
  • Feature(新特性)

Which issue(s) this PR fixes(Optional) (这个PR 修复了什么问题 (可选择))

  • resolve(修复) : # remove Address.sol, replace isContract with address(...).code.length > 0

What this PR does / why we need it (这个PR 做了什么/ 我们为什么需要这个PR)

to.isContract()报错

@XdpCs XdpCs self-requested a review November 5, 2024 06:52
@XdpCs
Copy link
Collaborator

XdpCs commented Nov 7, 2024

代码编译无法通过,最好不要使用内联汇编,这样会增加读者学习代码的成本

40_ERC1155/ERC1155.sol Show resolved Hide resolved
} else {
/// @solidity memory-safe-assembly
assembly {
revert(add(32, reason), mload(reason))
Copy link
Collaborator

Choose a reason for hiding this comment

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

内联汇编是否可以修改成可读性更高的

@XdpCs
Copy link
Collaborator

XdpCs commented Nov 7, 2024

如果大佬没空修改的话,明天我会在你这个分支上修改完,进行合并

@eddiehsu66
Copy link
Author

这个我回退一下版本就行,是merge Main的时候导致的错误,至于内联汇编哪里,我主要参考的是https://github.com/AmazingAng/WTF-Solidity/blob/main/34_ERC721/ERC721.sol
的实现
这个remove Address.sol也是因为34节的Address.sol找不到的缘故,估计是前面的某个PR被合并后,而40节却忘了修改的缘故。

@XdpCs
Copy link
Collaborator

XdpCs commented Nov 10, 2024

这个我回退一下版本就行,是merge Main的时候导致的错误,至于内联汇编哪里,我主要参考的是https://github.com/AmazingAng/WTF-Solidity/blob/main/34_ERC721/ERC721.sol 的实现 这个remove Address.sol也是因为34节的Address.sol找不到的缘故,估计是前面的某个PR被合并后,而40节却忘了修改的缘故。

好的 辛苦了 我最近比较忙

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