-
Notifications
You must be signed in to change notification settings - Fork 4
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
Introduce "IssueTokensFromGarage" action #295
Conversation
@@ -20,5 +23,14 @@ def long_format(self): | |||
def short_format(self): | |||
return self.raw.hex() | |||
|
|||
def derive(self, key: str) -> 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.
주소유도하는 메서드가 어디 따로 있었던걸로 기억하는데, 둘이 결과가 같을까요? 다르다면 고쳐야할것같고, 같다면 하나로 통일하는게 좋을것같습니다.
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.
네 저도 어디 만들어둔 것 같아서 찾아봤는데 안보여서 만들었습니다. 혹 정리하다 보이면 이쪽으로 모으는걸 생각하고 있습니다
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.
여기 에 있는데 return type 이 좀 다르네요. derive_address()
를 쓰는 곳은 garage address 유도를 위해 사용하는 곳 하나뿐인데, hash 를 만드는 것은 같고, return 할 때 마지막에 digest 하는 부분이 달라 이건 확인해서 통일해두겠습니다.
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.
새로 만든 코드로 통일할거라서 기존 코드에는 deprecate 표기 먼저 해놨습니다.
실제로 예전 버전 derive 를 사용하는 곳은 한곳인데, 이건 string > Address 로 타입 변경부터 크게 들어가야 해서 별도 이슈로 만들어두겠습니다.
@@ -179,8 +179,9 @@ def __init__(self, scope: Construct, construct_id: str, **kwargs) -> None: | |||
|
|||
if config.stage == "mainnet": | |||
ten_minute_event_rule.add_target(_event_targets.LambdaFunction(status_monitor)) | |||
else: | |||
hourly_event_rule.add_target(_event_targets.LambdaFunction(status_monitor)) | |||
# If you want to test monitor in internal, uncomment following statement |
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.
혹시 모니터링을 인터널에서 끄는 이유가 따로 있는걸까요?
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.
사실상 필요가 없는데 internal channel 에 noisy 하고 mainnet 과 헷갈리게 메세지가 와서 테스트가 필요한 기간이 아니면 끄면 좋겠다는 의견을 받았습니다
- Deprecate old function - Only one usage: need to be changed - Change new function - Use encode_checksum logic instead of hexdigest
- keys are not set - Update return values
@@ -24,6 +24,8 @@ on: | |||
required: true | |||
KMS_KEY_ID: | |||
required: true | |||
ADHOC_KMS_KEY_ID: |
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.
토큰 발행용 계정인걸까요? �ADHOC보단 별도의 정해진 이름이 있으면 좋을것같네요.(지금 당장 고쳐야할 건은 아닙니다.)
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.
네 garage > issue token 하는 계정입니다.
처음에 만든건 예전 IAP 초기에 수동지급 필요할 때 만들어서 adhoc 으로 한건데, 이름이야 뭐 고치면 됩니다 ㅋ
@@ -21,10 +21,13 @@ def fetch_secrets(region: str, secret_arn: str) -> Dict: | |||
return json.loads(resp["SecretString"]) | |||
|
|||
|
|||
def fetch_kms_key_id(stage: str, region: str) -> Optional[str]: | |||
def fetch_kms_key_id(stage: str, region: str, adhoc: bool = False) -> Optional[str]: |
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.
코드를 봐서는 서명용 키를 워커마다 다르게 가져가는 식인것같은데, 중복이 좀 되더라도 차라리 키별로 얻어오는 메서드를 아예 명시적으로 나눠버리는게 어떨까 싶네요.
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.
이걸 True 로 쓰는 데가 사실상 manual sign 하는 곳 (운영 계정에서 IssueToken + Transfer Assets) 밖에 없기는 한데
근데 저걸 또 가끔은 IAP 판매 계정으로 써야 할 때가 있어서요.
완전히 worker 별로 얘는 이거 쟤는 저거로 고정되어 있지는 않습니다.
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계정으로 B계정의 일을 처리하는게(혹은 반대) 겁나는 상황인건데, 이걸 막을수있을까요? (tx에 들어간 액션따라 허용 주소를 둔다던지)
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.
IssueTokensFromGarage
action 의 경우 무조건 context.Signer
주소를 가지고 작업을 하도록 되어 있어서 다른 계정의 재고를 건드리는 일은 없습니다.
그리고 우려하시는 A 계정의 garage > token 작업을 해야 할 때 B 계정으로 서명을 하는 경우에 대해서는
- 어차피 이제 모든 garage 는 token 으로 갈 거니 넘어가도 큰 문제는 안 된다
- 주소를 잘못 넣으면 garage 가 비어 있어서 에러가 날거다
정도가 발생할 것으로 보여서 크게 걱정은 안했습니다.
Derive
function to get derived addressIssueTokensFromGarage
action