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

Jump to bottom

feat(examples): grc20 refactor #2983

Open
wants to merge 26 commits into
base: master
Choose a base branch
from
Open

feat(examples): grc20 refactor #2983

wants to merge 26 commits into from

Conversation

moul
Copy link
Member

@moul moul commented Oct 19, 2024 •
edited
Loading

This PR extracts the grc20 refactor from #2551, which is a meta PR containing several contract improvements and additions that depend on new Gnovm features that haven't been merged yet.

Please review this grc20 refactor with a focus on its API. Several valuable comments can be found in #2551. Additionally, you can discover new contracts using grc20 in #2551, such as minidex, atomicswap, grc20reg, test20, and vault.

Addresses #1832

@moul moul self-assigned this Oct 19, 2024
@moul moul mentioned this pull request Oct 19, 2024
Draft
@github-actions github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label Oct 19, 2024
@codecov Codecov
Copy link

codecov bot commented Oct 19, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.36%. Comparing base (850182c) to head (2f83308).
Report is 5 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #2983      +/-   ##
==========================================
+ Coverage   63.11%   63.36%   +0.24%     
==========================================
  Files         548      548              
  Lines       78509    78878     +369     
==========================================
+ Hits        49550    49979     +429     
+ Misses      25612    25534      -78     
- Partials     3347     3365      +18
Flag Coverage Δ
contribs/gnodev 59.94% <ø> (ø)
contribs/gnofaucet 14.82% <ø> (ø)
gno.land 67.18% <ø> (+3.35%) ⬆️
gnovm 67.88% <ø> (+<0.01%) ⬆️
tm2 62.38% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@moul
chore: fixup
770d1ad 
Signed-off-by: moul <[email protected]>
@moul
chore: fixup
e2170f5 
Signed-off-by: moul <[email protected]>
@moul moul marked this pull request as ready for review October 19, 2024 01:32
@moul moul requested review from a team as code owners October 19, 2024 01:32
@moul moul requested review from jaekwon and gfanton and removed request for a team October 19, 2024 01:32
@leohhhn
Copy link
Contributor

leohhhn commented Oct 19, 2024

cc @MikaelVallenet @n0izn0iz, any thoughts?

@MikaelVallenet
Copy link
Contributor

cc @MikaelVallenet @n0izn0iz, any thoughts?

looks good to me, i like the teller pattern & add metadata in token level instead of ledger/bank
i'm just thinking if it should take another pkg as an extension of primitive grc20 pkg

thehowl
thehowl reviewed Oct 28, 2024
View reviewed changes
Copy link
Member

@thehowl thehowl left a comment

Choose a reason for hiding this comment

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

Still mostly naming considerations.

examples/gno.land/p/demo/grc/grc20/tellers.gno Outdated Show resolved Hide resolved
examples/gno.land/p/demo/grc/grc20/tellers.gno Outdated Show resolved Hide resolved
examples/gno.land/p/demo/grc/grc20/tellers.gno Outdated Show resolved Hide resolved
examples/gno.land/p/demo/grc/grc20/types.gno Outdated Show resolved Hide resolved
examples/gno.land/p/demo/grc/grc20/types.gno Outdated Show resolved Hide resolved
examples/gno.land/p/demo/grc/grc20/types.gno Outdated Show resolved Hide resolved
examples/gno.land/p/demo/grc/grc20/token.gno Outdated Show resolved Hide resolved
@Kouteki Kouteki added this to the 🚀 Mainnet launch milestone Oct 29, 2024
@Kouteki Kouteki removed the request for review from a team October 29, 2024 16:16
moul and others added 12 commits October 29, 2024 17:19
@moul @thehowl
Update examples/gno.land/p/demo/grc/grc20/tellers.gno
e92281f 
Co-authored-by: Morgan <[email protected]>
@moul @thehowl
Update examples/gno.land/p/demo/grc/grc20/tellers.gno
a656a51 
Co-authored-by: Morgan <[email protected]>
@moul
chore: fixup
a24c05f 
Signed-off-by: moul <[email protected]>
@moul
chore: fixup
6ea4b3c 
Signed-off-by: moul <[email protected]>
@moul
chore: fixup
6d763db 
Signed-off-by: moul <[email protected]>
@moul
chore: fixup
37e70d4 
Signed-off-by: moul <[email protected]>
@thehowl
chore: put replaces on gnolang/gno for all go.mods
9149395
@moul
chore: fixup
f129dc1 
Signed-off-by: moul <[email protected]>
@moul
Merge remote-tracking branch 'origin/master' into dev/moul/grc20-refacto
028156c
@thehowl
attempt to unfuck dockerfiles
f25e196
@thehowl
this hadnt been updated in a while
d7c4645
@moul
chore: fixup
0645b3d 
Signed-off-by: moul <[email protected]>
@github-actions github-actions bot added the 📦 🌐 tendermint v2 Issues or PRs tm2 related label Oct 29, 2024
@moul
Copy link
Member Author

moul commented Oct 29, 2024

Merged #3046 over this one to make the CI green.

Depends on #3046.

thehowl
thehowl approved these changes Oct 30, 2024
View reviewed changes
Copy link
Member

@thehowl thehowl left a comment

Choose a reason for hiding this comment

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

Overall implementation and naming LGTM.

examples/gno.land/p/demo/grc/grc20/tellers.gno Outdated Show resolved Hide resolved
examples/gno.land/p/demo/grc/grc20/tellers.gno Outdated Show resolved Hide resolved
examples/gno.land/r/demo/bar20/bar20.gno Outdated Show resolved Hide resolved
examples/gno.land/p/demo/grc/grc20/types.gno Outdated Show resolved Hide resolved
examples/gno.land/p/demo/grc/grc20/token.gno
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should have an issue to move the entirety of grc20 to use math/overflow, maybe after #2905.

@jefft0 jefft0 mentioned this pull request Oct 31, 2024
Closed
11 tasks
n0izn0iz
n0izn0iz suggested changes Oct 31, 2024
View reviewed changes
Copy link
Contributor

@n0izn0iz n0izn0iz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think this PR should also update the docs

@moul
Copy link
Member Author

moul commented Oct 31, 2024

@leohhhn do you want to update the doc in this PR or in your big refactor?

@thehowl thehowl mentioned this pull request Oct 31, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@n0izn0iz n0izn0iz n0izn0iz requested changes

@thehowl thehowl thehowl approved these changes

@jaekwon jaekwon Awaiting requested review from jaekwon jaekwon was automatically assigned from gnolang/tech-staff

@gfanton gfanton Awaiting requested review from gfanton gfanton was automatically assigned from gnolang/tech-staff

Assignees

@moul moul

Labels
📦 🌐 tendermint v2 Issues or PRs tm2 related 🧾 package/realm Tag used for new Realms or Packages.
Projects
💪 Bounties & Worx
Status: In Progress
😎 Manfred's Board
Status: 🎯 Current Topics
🧙‍♂️gno.land core team
Status: In Review
Milestone
🚀 Mainnet launch
Development

Successfully merging this pull request may close these issues.
6 participants
@moul @leohhhn @MikaelVallenet @thehowl @n0izn0iz @Kouteki