-
Notifications
You must be signed in to change notification settings - Fork 366
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
dev/moul/examples #2551
base: master
Are you sure you want to change the base?
dev/moul/examples #2551
Conversation
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
- create new user token | ||
- what about being seen by another contract as the prevRealm (dex) | ||
- wugnot | ||
- GOOD NAMES! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😄
Signed-off-by: moul <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea in grc20 seems solid overall. The names are confusing to work with; my ideal combination would be
AdminLedger
: the admin object containing all balancesToken
: a "safe" object that can be used as a variable to create Tellers. Implements GRC20 in read-only mode.Teller
: a way to access the Token, scoped to either theRealm
(RealmTeller
) or one of its subaccounts called "vaults" (VaultTeller
).
But I've added a bunch more ideas that came to mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe use a different account here than test1, so the coins don't conflict with those used to deploy the genesis pkgs.
} | ||
|
||
func NewBanker(name, symbol string, decimals uint) *Banker { | ||
func NewBank(name, symbol string, decimals uint) (*Bank, *AdminBanker) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it not make sense to just return an AdminBanker, from which the Bank can then be derived?
Kind of like you first generate a private key, then generate a public key from that.
name, symbol, decimals
should still be fixed (ie. initialized here, not with the AdminBanker.Bank()). Either we put the fields in the AdminBanker, or we can initialize the Bank and have it as a field of the AdminBanker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer this approach because you can generate your contract-related GRC20 token as a top-level variable in two ways:
import "p/grc20"
var Bank, admin = grc20.NewBank("...")
or like this:
import "p/grc20"
var _, Admin = grc20.NewBank("...")
I expect that most, if not all, contracts will eventually want to:
- expose the safe object publicly
- maintain a private instance of the admin for administrative tasks
This method allows for a single line of code for the most common initialization.
@@ -55,6 +71,17 @@ type Token interface { | |||
TransferFrom(from, to std.Address, amount uint64) error | |||
} | |||
|
|||
type fnBanker struct { | |||
accountFn func() std.Address | |||
bank *Bank |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could just embed *Bank
so it doesn't have to repeat the methods of Bank like GetName
, etc.
adm *AdminBanker | ||
} | ||
|
||
type AdminBanker struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideas for other names:
Ledger
. I already told you about this, but it doesn't match to how the term "ledger" is generally used in the blockchain context, so it's a bit weird.Mint
. This is relation to the physical facility. This is the one I like the most, even though it clashes with the methodMint
. Maybe that one can becomeIssue
?Tallies
/Balances
. So recalling the most important part kept in this structure.
I think the prefix Admin-
is worth keeping, as it clearly communicates that this should not be public.
) | ||
|
||
type Token interface { | ||
type Bank struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about Token
? I like it because if this is meant to be the "public object", then var Foo grc20.Token
reads nicely. Or maybe TokenInfo
(the "token" is not a data structure, but it is described by data structures).
Other idea, especially if we make this also compatible with the GRC20 interface readonly: make this a Teller
, then the account_banker.gno
functions RealmTeller
and VaultTeller
(ex AccountBanker
). (See other comments.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bank is closer to the std.Bank*
helpers. My goal with the rename was to create a more unified experience.
It is technically a bank—a place where you can handle your transactions, whether through self-service or by speaking with a teller.
} | ||
} | ||
|
||
func ReadonlyBanker(b *Bank) *fnBanker { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mentioned, I think the ReadonlyBanker could simply be the Bank
/ Token
itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ReadonlyBanker is similar to the Readonly Banker for an std.Coin. It is not about being "safe" or not; rather, it involves having (and being able to share) a pointer that can only be used for read-only operations.
We can have readonly safe objects and read-write safe objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Banker may be fine here, a few more options:
Wallet
. Communicates that it allows to do actions using the PrevRealm's bank account.Teller
. Like the Bank teller. It is an "interface" to make operations on the bank, but it is restricted and can only do stuff with a specified user's money.Spender
. Recalls the basic operation that can be done (spend money from a specific "account").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A banker is an "actor" who manages your money. This could be a teller, the person you authorize to access your bank account with specific limitations. Alternatively, you might choose to handle transactions yourself through self-service options. Essentially, a banker serves as an intermediary for token transactions.
When you have a wallet, you become your own banker. When you use a teller, you delegate your banking power to an external source. For example, if you give your credit card to a hotel that locks some funds for the duration of your stay, you make the hotel a potential limited spender.
"std" | ||
) | ||
|
||
func PrevRealmBanker(b *Bank) *fnBanker { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've always disliked returning concrete but unexported types, and I'd rather this returned a GRC20
interface.
Also, idea: just call this RealmBanker
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'll create interfaces instead, but probably not GRC20.
} | ||
|
||
func allowanceKey(owner, spender std.Address) string { | ||
return owner.String() + ":" + spender.String() | ||
} | ||
|
||
func AccountSlugAddr(addr std.Address, slug string) std.Address { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an implementation detail, should be unexported
} | ||
} | ||
|
||
func AccountBanker(b *Bank, slug string) *fnBanker { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also don't intuitively understand this one (before reading the implementation). Ideas:
- Simply, "Sub-account" to identify this is a child account scoped with the address of the parent.
- Revolut uses the word "Pocket" to talk about sub-accounts. Not particularly enthusiastic about tihs but worth considering.
- I've seen other places call them
Vault
. I like this one. - But we can also call it
Piggy
with reference to a piggy bank.
Blocked by #2743 |
@@ -0,0 +1,213 @@ | |||
package minidex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was this pkg supposed to be in p/
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, but I prefer having more r/ than p/ at the moment.
People are doing p/ that are not utilized. I definitely prefer the opposite.
Extracted from #2551 (also #2516). <!-- please provide a detailed description of the changes made in this pull request. --> <details><summary>Contributors' checklist...</summary> - [ ] Added new tests, or not needed, or not feasible - [ ] Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory - [ ] Updated the official documentation or not needed - [ ] No breaking changes were made, or a `BREAKING CHANGE: xxx` message was included in the description - [ ] Added references to related issues and PRs - [ ] Provided any useful hints for running manual tests - [ ] Added new benchmarks to [generated graphs](https://gnoland.github.io/benchmarks), if any. More info [here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md). </details> --------- Signed-off-by: moul <[email protected]> Co-authored-by: Morgan <[email protected]>
Extracted from #2551. See #2551 (comment). <!-- please provide a detailed description of the changes made in this pull request. --> <details><summary>Contributors' checklist...</summary> - [ ] Added new tests, or not needed, or not feasible - [ ] Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory - [ ] Updated the official documentation or not needed - [ ] No breaking changes were made, or a `BREAKING CHANGE: xxx` message was included in the description - [ ] Added references to related issues and PRs - [ ] Provided any useful hints for running manual tests - [ ] Added new benchmarks to [generated graphs](https://gnoland.github.io/benchmarks), if any. More info [here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md). </details> --------- Signed-off-by: moul <[email protected]>
Meta PR to verify that all my recent examples PR can work together.
This PR won't be merged but will be extracted into smaller independent PRs.
Note that the code depends on some not yet merged code like #2535, and this PR may not be merged in favor of another method. In other words, this PR is not fully compatible with
master
and will eventually never be compatible without some changes.Some related PRs: