[ans] set subdomain renew policy #53
Conversation
angieyth
force-pushed
the
angie/subdomain_policy
branch
from
4312a6f
to
d602965
Compare
core_v2/sources/domains.move
Outdated
| @@ -31,6 +31,10 @@ module aptos_names_v2::domains { | |||
| const AUTO_RENEWAL_EXPIRATION_CUTOFF_SEC: u64 = 1709855999; | |||
| const SECONDS_PER_YEAR: u64 = 60 * 60 * 24 * 365; | |||
|
|
|||
| /// enums for subdomain expiration policy | |||
| const SUBDOMAIN_POLICY_LOOKUP_DOMAIN_EXPIRATION: u8 = 0; | |||
There was a problem hiding this comment.
is this the right default? I feel like SUBDOMAIN_POLICY_LOOKUP_DOMAIN_EXPIRATION is a non-default policy because if we follow the way domain expiration works, manual expiration is the expected expiration policy.
There was a problem hiding this comment.
I'm going to change this as a param passing in after this PR. no default behavior anymore
There was a problem hiding this comment.
nit I personally prefer if we have SUBDOMAIN_POLICY_MANUAL_SET_EXPIRATION = 0 because that was the original behavior in v1
core_v2/sources/domains.move
Outdated
| } | ||
|
|
||
| fun validate_subdomain_expiration_policy( | ||
| use_domain_expiration_sec: u8, |
There was a problem hiding this comment.
rename: subdomain_expiration_policy
| test_helper::register_name(user, option::none(), test_helper::domain_name(), test_helper::one_year_secs(), test_helper::fq_domain_name(), 1, vector::empty<u8>()); | ||
|
|
||
| // Register a subdomain! | ||
| test_helper::register_name(user, option::some(test_helper::subdomain_name()), test_helper::domain_name(), test_helper::one_year_secs(), test_helper::fq_subdomain_name(), 1, vector::empty<u8>()); |
There was a problem hiding this comment.
assert the default policy before change as well
| @@ -679,7 +685,7 @@ module aptos_names_v2::domains { | |||
| let record = get_record_mut(domain_name, option::some(subdomain_name)); | |||
| assert!(option::is_some(&record.subdomain_ext), error::invalid_state(ENOT_A_SUBDOMAIN)); | |||
| let subdomain_ext = option::borrow(&record.subdomain_ext); | |||
| if (subdomain_ext.use_domain_expiration_sec) { | |||
| if (subdomain_ext.subdomain_expiration_policy == SUBDOMAIN_POLICY_LOOKUP_DOMAIN_EXPIRATION) { | |||
| assert!(false, error::invalid_state(ESUBDOMAIN_IS_AUTO_RENEW)); | |||
There was a problem hiding this comment.
should we allow set, even if the policy doesn't use the expiration?
What if the domain owner wants to change the expiration before they turn off the auto-renew flag?
There was a problem hiding this comment.
if auto-renew flag is on and user wants to set the expiration date, we can either
- let them set and implicitly switch the flag back
- throw error because the flag is on
I like the second one better because there's no extra action implied. I don't feel strong on either one though
There was a problem hiding this comment.
I don't feel strongly either. I think let's just do 2. for now and we can always upgrade the contract later if we need to change the behavior.
core_v2/sources/domains.move
Outdated
| @@ -31,6 +31,10 @@ module aptos_names_v2::domains { | |||
| const AUTO_RENEWAL_EXPIRATION_CUTOFF_SEC: u64 = 1709855999; | |||
| const SECONDS_PER_YEAR: u64 = 60 * 60 * 24 * 365; | |||
|
|
|||
| /// enums for subdomain expiration policy | |||
There was a problem hiding this comment.
can we add a comment to update validate_subdomain_expiration_policy if adding a new enum?
Maybe also good idea to have // const SUBDOMAIN_POLICY_NEXT_ENUM = 2 comment so that we keep track of what the next number should be.
angieyth
force-pushed
the
angie/subdomain_policy
branch
from
e664acd
to
ca00721
Compare
test_