-
Notifications
You must be signed in to change notification settings - Fork 25
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
Allow setting target address and transfer to address during registration #59
Conversation
3cc7885
to
d84800e
Compare
do you think we should add an unit test for transfer and another one for target address? (I sound like brian :P) |
I should, im figuring out how, your are right unit test is the most painful thing |
0e4efee
to
1adef50
Compare
70cfca4
to
4310171
Compare
// Automatically set the name to point to the sender's address | ||
set_target_address_internal(subdomain_name, domain_name, signer::address_of(sign)); | ||
// If the signer has no reverse lookup set and signer is minting for itself, set the user's reverse lookup and target address. | ||
if (result_of_should_set_reverse_lookup_and_target_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.
is it possible to separate the logic of reverse look up and set target address in should_set_reverse_lookup_and_target_address_in_register_name
I see we also have should_set_target_address_in_register_name
and it's confusing
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.
it's because set reverse lookup will always set target address at the same time, if i break it into 2, i need to break set_target_address_and_reverse_lookup
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 do think we should break set_target_address_and_reverse_lookup
:P
because those are basically two different things. aren't them?
you set primary name when the buyer is the owner
you set target address whenever there's a value passing in?
// Automatically set the name to point to the sender's address | ||
set_target_address_internal(subdomain_name, domain_name, signer::address_of(sign)); | ||
// If the signer has no reverse lookup set and signer is minting for itself, set the user's reverse lookup and target address. | ||
if (result_of_should_set_reverse_lookup_and_target_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.
I do think we should break set_target_address_and_reverse_lookup
:P
because those are basically two different things. aren't them?
you set primary name when the buyer is the owner
you set target address whenever there's a value passing in?
core_v2/sources/domains.move
Outdated
target_address: Option<address>, | ||
): bool { | ||
if (!is_subdomain(subdomain_name)) { | ||
// If signer is registering a domain, automatically set the name to point to target address, if not set use signer 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.
I don't understand this case, can you elaborate?
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.
updated the comment, basically when registering a domain, we always set the target address of the new domain
Summary
set_reverse_lookup
toset_name_address_and_reverse_lookup
to avoid confusion.set_name_address_and_reverse_lookup
should_set_reverse_lookup_and_target_addres_result
, this returns true only when signer set both target address and transfer to address to itself, then we set primary name