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

Solidity imported libraries should have a version #643

Closed
eugenioclrc opened this issue May 18, 2023 · 38 comments · Fixed by #661 or #687
Closed

Solidity imported libraries should have a version #643

eugenioclrc opened this issue May 18, 2023 · 38 comments · Fixed by #661 or #687

Comments

@eugenioclrc
Copy link
Contributor

eugenioclrc commented May 18, 2023

Currently its not clear what version of used libraries the protocolo is using;
https://github.com/ubiquity/ubiquity-dollar/blob/development/.gitmodules

What should be done:

  1. Remove tags (because they are not working in git submodules) and add branch names to git submodules (perhaps this requires some code refactoring, make sure that libs are also updated after settings branch names)
  2. Remove duplicate entries in git submodules
@0x4007
Copy link
Member

0x4007 commented May 18, 2023

@rndquu @zgorizzo69 any inputs on this?

@eugenioclrc
Copy link
Contributor Author

@rndquu @zgorizzo69 any inputs on this?

I could take it but first I want to hear your opinions

This was referenced May 18, 2023
@rndquu
Copy link
Member

rndquu commented May 19, 2023

@eugenioclrc

So in the end in git submodules we will have smth similar to:

[submodule "packages/contracts/lib/openzeppelin-contracts"]
	path = packages/contracts/lib/openzeppelin-contracts
	url = https://github.com/OpenZeppelin/openzeppelin-contracts
        branch = release-v4.9
[submodule "packages/contracts/lib/forge-std"]
	path = packages/contracts/lib/forge-std
	url = https://github.com/foundry-rs/forge-std
	branch = v1

Questions:

  1. Is the above example correct?
  2. This way we will no more have the ability to use forge update but use only git submodule update --remote, right? (in this case it would be nice to have a separate script in package.json like forge:update that uses git submodule update --remote under the hood)

Also some dependencies may be broken after such an update so perhaps this task also requires some refactoring.

Overall the issue looks good.

@eugenioclrc
Copy link
Contributor Author

@eugenioclrc

So in the end in git submodules we will have smth similar to:

[submodule "packages/contracts/lib/openzeppelin-contracts"]
	path = packages/contracts/lib/openzeppelin-contracts
	url = https://github.com/OpenZeppelin/openzeppelin-contracts
        branch = release-v4.9
[submodule "packages/contracts/lib/forge-std"]
	path = packages/contracts/lib/forge-std
	url = https://github.com/foundry-rs/forge-std
	branch = v1

Questions:

  1. Is the above example correct?
  2. This way we will no more have the ability to use forge update but use only git submodule update --remote, right? (in this case it would be nice to have a separate script in package.json like forge:update that uses git submodule update --remote under the hood)

Also some dependencies may be broken after such an update so perhaps this task also requires some refactoring.

Overall the issue looks good.

  1. Yes, you are correct
  2. i think that instead of forge update a simple forge install should be enough, and if you want to update the lib you just change the branch, another suggestion could be using package.json to handle the libraries. This is an example of a project using that config; package.json#L25-L31

@rndquu
Copy link
Member

rndquu commented May 19, 2023

another suggestion could be using package.json to handle the libraries

I think setting branch name in git submodules should be enough at the current project stage.

Migrating to package.json has some drawbacks:

  1. Not a "native" way to install dependencies in foundry framework
  2. New devs who come to the project would expect dependencies in the lib folder (because this is a foundry project)
  3. Something should definitely be broken (some CI actions or scripts) if we remove the lib folder with git submodules

@PhantomCracker
Copy link
Contributor

PhantomCracker commented May 28, 2023

/start

@PhantomCracker
Copy link
Contributor

Assign does not work anymore? Or it is because another PR was merged? (from what i have checked, the other PR does not solve this issue)

@ubiquibot
Copy link

ubiquibot bot commented May 28, 2023

Skipping /start since the issue is closed

@0x4007
Copy link
Member

0x4007 commented May 28, 2023

We just pushed a big update to the bot and there are some big stability issues at the moment - sorry about that @PhantomCracker.

@0xcodercrane

@0xcodercrane
Copy link
Contributor

big apologize here @PhantomCracker

@PhantomCracker
Copy link
Contributor

big apologize here @PhantomCracker

No worries 👍

0x4007 added a commit that referenced this issue May 31, 2023
#643 - Solidity imported libraries should have a tagged version
@ubiquibot ubiquibot bot added the Permitted label May 31, 2023
@ubiquibot
Copy link

ubiquibot bot commented May 31, 2023

[ CLAIM 50 DAI ]

0xCAC0A8c...9AF40A6

@rndquu
Copy link
Member

rndquu commented Jun 2, 2023

@PhantomCracker hi, I'm trying to reproduce the update to specific lib versions and it seems that git submodule update --init --remote updates only to the latest lib version ignoring any git submodule tags

So here is a list of improvements:

  1. Git submodule tags do not a affect the updated lib version. We should use an exact branch name instead
  2. We keep using the old libs here. This task implied all dependencies to be updated to the latest version.

Could you fix those issues?

@rndquu rndquu reopened this Jun 2, 2023
@PhantomCracker
Copy link
Contributor

@rndquu I made this commit for this pull request, can you please check it?
d554cbd
Also, if we include both branch and tags, tags have priority over branches

@rndquu rndquu changed the title Solidity imported libraries should have a tagged version Solidity imported libraries should have a version Jun 19, 2023
@ghost
Copy link

ghost commented Jun 19, 2023

/start

@ubiquibot
Copy link

ubiquibot bot commented Jun 19, 2023

Skipping /start since the issue is closed

@ghost
Copy link

ghost commented Jun 19, 2023

/start

@ubiquibot ubiquibot bot assigned ghost Jun 19, 2023
@ubiquibot
Copy link

ubiquibot bot commented Jun 19, 2023

Skipping /start since the issue is closed

@eugenioclrc
Copy link
Contributor Author

eugenioclrc commented Jun 19, 2023

/start

@ubiquibot
Copy link

ubiquibot bot commented Jun 19, 2023

Skipping /start since the issue is closed

@0x4007
Copy link
Member

0x4007 commented Jun 19, 2023

Looks like a few hunters are trying to hunt this bounty. @rndquu can you take this reviewing over and make sure thats its handled correctly?

@ubiquibot
Copy link

ubiquibot bot commented Jun 26, 2023

[ CLAIM 200 WXDAI ]

0x2bBc6a83...b75E3708c

@0x4007
Copy link
Member

0x4007 commented Jun 26, 2023

Looks like a few hunters are trying to hunt this bounty. @rndquu can you take this reviewing over and make sure thats its handled correctly?

Congratulations @AnakinSkywalkeer you prevailed as the strongest hunter.

@0x4007
Copy link
Member

0x4007 commented Jun 26, 2023

@0xcodercrane it looks like we generated two payment permits for this issue. Shouldn't this not be possible?

@ghost
Copy link

ghost commented Jun 26, 2023

Looks like a few hunters are trying to hunt this bounty. @rndquu can you take this reviewing over and make sure thats its handled correctly?

Congratulations @AnakinSkywalkeer you prevailed as the strongest hunter.

Cool! :)

@ghost
Copy link

ghost commented Jun 26, 2023

[ CLAIM 200 WXDAI ]

0x2bBc6a83...b75E3708c

@pavlovcik hey can u help me with this like i conected my metamask then i clicked claim it gave me
"PERMIT ALREADY CLAIMED" but i didn't receive 🤔

@rndquu
Copy link
Member

rndquu commented Jun 26, 2023

[ CLAIM 200 WXDAI ]

0x2bBc6a83...b75E3708c

@pavlovcik hey can u help me with this like i conected my metamask then i clicked claim it gave me "PERMIT ALREADY CLAIMED" but i didn't receive 🤔

What happened:

  1. Originally @PhantomCracker solved the current issue
  2. Permit was generated
  3. Then the issue was opened again because of the errors
  4. @AnakinSkywalkeer solved the issue correctly
  5. New permit was generated

Permits from step 2 and 5 have the same nonce. Permit from step 2 has already been claimed so permit from step 5 is invalid.

@pavlovcik help

@ghost
Copy link

ghost commented Jun 26, 2023

[ CLAIM 200 WXDAI ]

0x2bBc6a83...b75E3708c

@pavlovcik hey can u help me with this like i conected my metamask then i clicked claim it gave me "PERMIT ALREADY CLAIMED" but i didn't receive 🤔

What happened:

  1. Originally @PhantomCracker solved the current issue
  2. Permit was generated
  3. Then the issue was opened again because of the errors
  4. @AnakinSkywalkeer solved the issue correctly
  5. New permit was generated

Permits from step 2 and 5 have the same nonce. Permit from step 2 has already been claimed so permit from step 5 is invalid.

@pavlovcik help

ohhh

@PhantomCracker
Copy link
Contributor

Maybe we need to create a new issue every time an error occurs instead of reopen the old ones?

P.S: Good job @AnakinSkywalkeer

@0x4007
Copy link
Member

0x4007 commented Jun 26, 2023

Maybe we need to create a new issue every time an error occurs instead of reopen the old ones?

This was not implemented correctly the first time around so it should not have been accepted and paid out.


@AnakinSkywalkeer 0x2bBc6a8314044d70684C74CfA0E117eb75E3708c can you verify that this is your address?

@ghost
Copy link

ghost commented Jun 26, 2023

Maybe we need to create a new issue every time an error occurs instead of reopen the old ones?

This was not implemented correctly the first time around so it should not have been accepted and paid out.

@AnakinSkywalkeer 0x2bBc6a8314044d70684C74CfA0E117eb75E3708c can you verify that this is your address?

yes its my address 0x2bBc6a8314044d70684C74CfA0E117eb75E3708c

@0x4007
Copy link
Member

0x4007 commented Jun 26, 2023

[ CLAIM 200 WXDAI ]

0x2bBc6a83...b75E3708c

@pavlovcik hey can u help me with this like i conected my metamask then i clicked claim it gave me "PERMIT ALREADY CLAIMED" but i didn't receive 🤔

What happened:

  1. Originally @PhantomCracker solved the current issue
  2. Permit was generated
  3. Then the issue was opened again because of the errors
  4. @AnakinSkywalkeer solved the issue correctly
  5. New permit was generated

Permits from step 2 and 5 have the same nonce. Permit from step 2 has already been claimed so permit from step 5 is invalid.

@pavlovcik help

@rndquu shouldn't it show "permit already claimed" as soon as the page loads? I remember seeing claimed permits being recognized with no user interaction on the dApp.

@0x4007
Copy link
Member

0x4007 commented Jun 26, 2023

@eugenioclrc
Copy link
Contributor Author

eugenioclrc commented Jun 27, 2023 via email

@ghost
Copy link

ghost commented Jun 27, 2023

@0x4007
Copy link
Member

0x4007 commented Jun 30, 2023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants